Skip to content

Commit 8155d08

Browse files
nate-chandlernate-chandler
authored
Merge pull request #64614 from nate-chandler/rdar107198526
[CopyPropagation] Fix destroy recording.
2 parents efb7f24 + 7493b7e commit 8155d08

File tree

2 files changed

+133
-39
lines changed

2 files changed

+133
-39
lines changed

‎lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp

Lines changed: 63 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -101,19 +101,22 @@ static void diagnose(ASTContext &Context, SourceLoc loc, Diag<T...> diag,
101101
Context.Diags.diagnose(loc, diag, std::forward<U>(args)...);
102102
}
103103

104-
static DestroyValueInst *dynCastToDestroyOf(SILInstruction *instruction,
105-
SILValue def) {
104+
/// Is \p instruction a destroy_value whose operand is \p def, or its
105+
/// transitive copy.
106+
static bool isDestroyOfCopyOf(SILInstruction *instruction, SILValue def) {
106107
auto *destroy = dyn_cast<DestroyValueInst>(instruction);
107108
if (!destroy)
108-
return nullptr;
109-
auto originalDestroyedDef = destroy->getOperand();
110-
if (originalDestroyedDef == def)
111-
return destroy;
112-
auto underlyingDestroyedDef =
113-
CanonicalizeOSSALifetime::getCanonicalCopiedDef(originalDestroyedDef);
114-
if (underlyingDestroyedDef != def)
115-
return nullptr;
116-
return destroy;
109+
return false;
110+
auto destroyed = destroy->getOperand();
111+
while (true) {
112+
if (destroyed == def)
113+
return true;
114+
auto *copy = dyn_cast<CopyValueInst>(destroyed);
115+
if (!copy)
116+
break;
117+
destroyed = copy->getOperand();
118+
}
119+
return false;
117120
}
118121

119122
//===----------------------------------------------------------------------===//
@@ -172,11 +175,18 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
172175
liveness->updateForUse(user, /*lifetimeEnding*/ true);
173176
break;
174177
case OperandOwnership::DestroyingConsume:
175-
if (isa<DestroyValueInst>(user)) {
178+
if (isDestroyOfCopyOf(user, getCurrentDef())) {
176179
destroys.insert(user);
177180
} else {
178-
// destroy_value does not force pruned liveness (but store etc. does).
179-
liveness->updateForUse(user, /*lifetimeEnding*/ true);
181+
// destroy_value of a transitive copy of the currentDef does not
182+
// force pruned liveness (but store etc. does).
183+
184+
// Even though this instruction is a DestroyingConsume of its operand,
185+
// if it's a destroy_value whose operand is not a transitive copy of
186+
// currentDef, then it's just ending an implicit borrow of currentDef,
187+
// not consuming it.
188+
auto lifetimeEnding = !isa<DestroyValueInst>(user);
189+
liveness->updateForUse(user, lifetimeEnding);
180190
}
181191
recordConsumingUse(use);
182192
break;
@@ -511,7 +521,7 @@ void CanonicalizeOSSALifetime::extendUnconsumedLiveness(
511521
// destroys.
512522
BasicBlockWorklist worklist(currentDef->getFunction());
513523
for (auto *instruction : boundary.lastUsers) {
514-
if (dynCastToDestroyOf(instruction, getCurrentDef()))
524+
if (destroys.contains(instruction))
515525
continue;
516526
if (liveness->isInterestingUser(instruction)
517527
!= PrunedLiveness::IsInterestingUser::LifetimeEndingUse)
@@ -576,17 +586,20 @@ namespace {
576586
/// values with overlapping live ranges and failing to find a fixed point
577587
/// because their destroys are repeatedly hoisted over one another.
578588
class ExtendBoundaryToDestroys final {
589+
using InstructionPredicate = llvm::function_ref<bool(SILInstruction *)>;
579590
SSAPrunedLiveness &liveness;
580591
PrunedLivenessBoundary const &originalBoundary;
581592
SILValue currentDef;
582593
BasicBlockSet seenMergePoints;
594+
InstructionPredicate isDestroy;
583595

584596
public:
585597
ExtendBoundaryToDestroys(SSAPrunedLiveness &liveness,
586598
PrunedLivenessBoundary const &originalBoundary,
587-
SILValue currentDef)
599+
SILValue currentDef, InstructionPredicate isDestroy)
588600
: liveness(liveness), originalBoundary(originalBoundary),
589-
currentDef(currentDef), seenMergePoints(currentDef->getFunction()){};
601+
currentDef(currentDef), seenMergePoints(currentDef->getFunction()),
602+
isDestroy(isDestroy){};
590603
ExtendBoundaryToDestroys(ExtendBoundaryToDestroys const &) = delete;
591604
ExtendBoundaryToDestroys &
592605
operator=(ExtendBoundaryToDestroys const &) = delete;
@@ -610,34 +623,37 @@ class ExtendBoundaryToDestroys final {
610623
/// Look past ignoreable instructions to find the _last_ destroy after the
611624
/// specified instruction that destroys \p def.
612625
static DestroyValueInst *findDestroyAfter(SILInstruction *previous,
613-
SILValue def) {
626+
SILValue def,
627+
InstructionPredicate isDestroy) {
614628
DestroyValueInst *retval = nullptr;
615629
for (auto *instruction = previous->getNextInstruction(); instruction;
616630
instruction = instruction->getNextInstruction()) {
617631
if (!CanonicalizeOSSALifetime::ignoredByDestroyHoisting(
618632
instruction->getKind()))
619633
break;
620-
if (auto destroy = dynCastToDestroyOf(instruction, def))
621-
retval = destroy;
634+
if (isDestroy(instruction))
635+
retval = cast<DestroyValueInst>(instruction);
622636
}
623637
return retval;
624638
}
625639

626640
/// Look past ignoreable instructions to find the _last_ destroy at or after
627641
/// the specified instruction that destroys \p def.
628-
static DestroyValueInst *findDestroyAtOrAfter(SILInstruction *start,
629-
SILValue def) {
630-
if (auto *dvi = dynCastToDestroyOf(start, def))
631-
return dvi;
632-
return findDestroyAfter(start, def);
642+
static DestroyValueInst *
643+
findDestroyAtOrAfter(SILInstruction *start, SILValue def,
644+
InstructionPredicate isDestroy) {
645+
if (isDestroy(start))
646+
return cast<DestroyValueInst>(start);
647+
return findDestroyAfter(start, def, isDestroy);
633648
}
634649

635650
/// Look past ignoreable instructions to find the _first_ destroy in \p
636651
/// destination that destroys \p def and isn't separated from the beginning
637652
/// by "interesting" instructions.
638-
static DestroyValueInst *findDestroyFromBlockBegin(SILBasicBlock *destination,
639-
SILValue def) {
640-
return findDestroyAtOrAfter(&*destination->begin(), def);
653+
static DestroyValueInst *
654+
findDestroyFromBlockBegin(SILBasicBlock *destination, SILValue def,
655+
InstructionPredicate isDestroy) {
656+
return findDestroyAtOrAfter(&*destination->begin(), def, isDestroy);
641657
}
642658

643659
private:
@@ -651,12 +667,14 @@ class ExtendBoundaryToDestroys final {
651667
/// stays in place and \p def remains a dead def.
652668
void extendBoundaryFromDef(SILNode *def, PrunedLivenessBoundary &boundary) {
653669
if (auto *arg = dyn_cast<SILArgument>(def)) {
654-
if (auto *dvi = findDestroyFromBlockBegin(arg->getParent(), currentDef)) {
670+
if (auto *dvi = findDestroyFromBlockBegin(arg->getParent(), currentDef,
671+
isDestroy)) {
655672
boundary.lastUsers.push_back(dvi);
656673
return;
657674
}
658675
} else {
659-
if (auto *dvi = findDestroyAfter(cast<SILInstruction>(def), currentDef)) {
676+
if (auto *dvi = findDestroyAfter(cast<SILInstruction>(def), currentDef,
677+
isDestroy)) {
660678
boundary.lastUsers.push_back(dvi);
661679
return;
662680
}
@@ -673,7 +691,8 @@ class ExtendBoundaryToDestroys final {
673691
/// stays in place and \p destination remains a boundary edge.
674692
void extendBoundaryFromBoundaryEdge(SILBasicBlock *destination,
675693
PrunedLivenessBoundary &boundary) {
676-
if (auto *dvi = findDestroyFromBlockBegin(destination, currentDef)) {
694+
if (auto *dvi =
695+
findDestroyFromBlockBegin(destination, currentDef, isDestroy)) {
677696
boundary.lastUsers.push_back(dvi);
678697
} else {
679698
boundary.boundaryEdges.push_back(destination);
@@ -694,8 +713,9 @@ class ExtendBoundaryToDestroys final {
694713
/// user remains a last user.
695714
void extendBoundaryFromUser(SILInstruction *user,
696715
PrunedLivenessBoundary &boundary) {
697-
if (auto *dvi = dynCastToDestroyOf(user, currentDef)) {
698-
auto *existingDestroy = findDestroyAtOrAfter(dvi, currentDef);
716+
if (isDestroy(user)) {
717+
auto *dvi = cast<DestroyValueInst>(user);
718+
auto *existingDestroy = findDestroyAtOrAfter(dvi, currentDef, isDestroy);
699719
assert(existingDestroy && "couldn't find a destroy at or after one!?");
700720
boundary.lastUsers.push_back(existingDestroy);
701721
return;
@@ -713,7 +733,8 @@ class ExtendBoundaryToDestroys final {
713733
extendBoundaryFromTerminator(terminator, boundary);
714734
return;
715735
}
716-
if (auto *existingDestroy = findDestroyAfter(user, currentDef)) {
736+
if (auto *existingDestroy =
737+
findDestroyAfter(user, currentDef, isDestroy)) {
717738
boundary.lastUsers.push_back(existingDestroy);
718739
return;
719740
}
@@ -745,7 +766,8 @@ class ExtendBoundaryToDestroys final {
745766
assert(block->getSingleSuccessorBlock() == successor);
746767
continue;
747768
}
748-
if (auto *dvi = findDestroyFromBlockBegin(successor, currentDef)) {
769+
if (auto *dvi =
770+
findDestroyFromBlockBegin(successor, currentDef, isDestroy)) {
749771
boundary.lastUsers.push_back(dvi);
750772
foundDestroy = true;
751773
} else {
@@ -770,8 +792,9 @@ void CanonicalizeOSSALifetime::findExtendedBoundary(
770792
PrunedLivenessBoundary &boundary) {
771793
assert(boundary.lastUsers.size() == 0 && boundary.boundaryEdges.size() == 0 &&
772794
boundary.deadDefs.size() == 0);
795+
auto isDestroy = [&](auto *inst) { return destroys.contains(inst); };
773796
ExtendBoundaryToDestroys extender(*liveness, originalBoundary,
774-
getCurrentDef());
797+
getCurrentDef(), isDestroy);
775798
extender.extend(boundary);
776799
}
777800

@@ -806,8 +829,8 @@ void CanonicalizeOSSALifetime::insertDestroysOnBoundary(
806829
PrunedLivenessBoundary const &boundary) {
807830
BasicBlockSet seenMergePoints(getCurrentDef()->getFunction());
808831
for (auto *instruction : boundary.lastUsers) {
809-
if (auto *dvi = dynCastToDestroyOf(instruction, getCurrentDef())) {
810-
consumes.recordFinalConsume(dvi);
832+
if (destroys.contains(instruction)) {
833+
consumes.recordFinalConsume(instruction);
811834
continue;
812835
}
813836
switch (liveness->isInterestingUser(instruction)) {
@@ -905,7 +928,8 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
905928
defUseWorklist.insert(copy);
906929
return true;
907930
}
908-
if (auto *destroy = dynCastToDestroyOf(user, getCurrentDef())) {
931+
if (destroys.contains(user)) {
932+
auto *destroy = cast<DestroyValueInst>(user);
909933
// If this destroy was marked as a final destroy, ignore it; otherwise,
910934
// delete it.
911935
if (!consumes.claimConsume(destroy)) {

‎test/SILOptimizer/copy_propagation_value.sil

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// RUN: %target-sil-opt -copy-propagation -enable-sil-verify-all -module-name Swift %s | %FileCheck %s --check-prefixes=CHECK,CHECK-OPT
2+
// RUN: %target-sil-opt -mandatory-copy-propagation -enable-sil-verify-all -module-name Swift %s | %FileCheck %s --check-prefixes=CHECK,CHECK-ONONE
3+
4+
// Runs CopyPropagation without borrow scope canonicalization.
5+
6+
sil_stage canonical
7+
8+
import Builtin
9+
10+
typealias AnyObject = Builtin.AnyObject
11+
12+
protocol Error {}
13+
14+
class B { }
15+
16+
class C {
17+
var a: Builtin.Int64
18+
}
19+
20+
struct NonTrivialStruct {
21+
@_hasStorage var val: C { get set }
22+
}
23+
24+
class CompileError : Error {}
25+
26+
// This test case used to have an invalid boundary extension.
27+
// CHECK-LABEL: sil [ossa] @canonicalize_borrow_of_copy_with_interesting_boundary : $@convention(thin) (@owned C) -> (@owned NonTrivialStruct, @error any Error) {
28+
// CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] :
29+
// CHECK: [[COPY:%[^,]+]] = copy_value [[INSTANCE]]
30+
// CHECK: destroy_value [[INSTANCE]]
31+
// CHECK: cond_br undef, [[SUCCESS:bb[0-9]+]], [[FAILURE:bb[0-9]+]]
32+
// CHECK: [[SUCCESS]]:
33+
// CHECK: [[BORROW:%[^,]+]] = begin_borrow [[COPY]]
34+
// CHECK: [[STRUCT:%[^,]+]] = struct $NonTrivialStruct ([[BORROW]] : $C)
35+
// CHECK: [[STRUCT_OUT:%[^,]+]] = copy_value [[STRUCT]]
36+
// CHECK: end_borrow [[BORROW]]
37+
// CHECK: destroy_value [[COPY]]
38+
// CHECK: return [[STRUCT_OUT]]
39+
// CHECK: [[FAILURE]]:
40+
// CHECK: destroy_value [[COPY]]
41+
// CHECK: [[BOX:%[^,]+]] = alloc_existential_box
42+
// CHECK: throw [[BOX]]
43+
// CHECK-LABEL: } // end sil function 'canonicalize_borrow_of_copy_with_interesting_boundary'
44+
sil [ossa] @canonicalize_borrow_of_copy_with_interesting_boundary : $@convention(thin) (@owned C) -> (@owned NonTrivialStruct, @error any Error) {
45+
bb0(%0 : @owned $C):
46+
%1 = copy_value %0 : $C
47+
%2 = copy_value %1 : $C
48+
cond_br undef, bb1, bb2
49+
bb1:
50+
destroy_value %0 : $C
51+
destroy_value %1 : $C
52+
%6 = begin_borrow %2 : $C
53+
%7 = struct $NonTrivialStruct (%6 : $C)
54+
%8 = copy_value %7 : $NonTrivialStruct
55+
end_borrow %6 : $C
56+
destroy_value %2 : $C
57+
return %8 : $NonTrivialStruct
58+
bb2:
59+
destroy_value %2 : $C
60+
%13 = begin_borrow %1 : $C
61+
%14 = struct $NonTrivialStruct (%13 : $C)
62+
%15 = copy_value %14 : $NonTrivialStruct
63+
end_borrow %13 : $C
64+
destroy_value %1 : $C
65+
destroy_value %15 : $NonTrivialStruct
66+
%19 = alloc_existential_box $any Error, $CompileError
67+
destroy_value %0 : $C
68+
%22 = builtin "willThrow"(%19 : $any Error) : $()
69+
throw %19 : $any Error
70+
}

0 commit comments

Comments
 (0)