Skip to content

Commit e45bb6f

Browse files
swift-ciswift-ci
authored
Merge pull request #41460 from atrick/fix-canonical-func-arg
Fix an assert in canonicalizeFunctionArgument.
2 parents 004bc8a + 4a30118 commit e45bb6f

File tree

4 files changed

+146
-75
lines changed

4 files changed

+146
-75
lines changed

‎lib/SILOptimizer/Transforms/CopyPropagation.cpp

Lines changed: 62 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -89,20 +89,23 @@ struct CanonicalDefWorklist {
8989

9090
while (true) {
9191
def = CanonicalizeOSSALifetime::getCanonicalCopiedDef(def);
92-
if (!canonicalizeBorrows) {
93-
ownedValues.insert(def);
94-
return;
95-
}
92+
9693
// If the copy's source is guaranteed, find the root of a borrowed
9794
// extended lifetime.
9895
if (auto *copy = dyn_cast<CopyValueInst>(def)) {
9996
if (SILValue borrowDef =
10097
CanonicalizeBorrowScope::getCanonicalBorrowedDef(
10198
copy->getOperand())) {
102-
borrowedValues.insert(borrowDef);
103-
return;
99+
if (canonicalizeBorrows || isa<SILFunctionArgument>(borrowDef)) {
100+
borrowedValues.insert(borrowDef);
101+
return;
102+
}
104103
}
105104
}
105+
if (!canonicalizeBorrows) {
106+
ownedValues.insert(def);
107+
return;
108+
}
106109
// Look through hoistable owned forwarding instructions on the
107110
// use-def chain.
108111
if (SILInstruction *defInst = def->getDefiningInstruction()) {
@@ -491,60 +494,63 @@ void CopyPropagation::run() {
491494
// of the copiedDefs. If the are converted, they are removed from copiedDefs
492495
// and the source of the new destructure is added.
493496
changed |= convertExtractsToDestructures(defWorklist, deleter);
494-
495-
// borrowCanonicalizer performs all modifications through deleter's
496-
// callbacks, so we don't need to explicitly check for changes.
497-
CanonicalizeBorrowScope borrowCanonicalizer(deleter);
498-
// The utilities in this loop cannot delete borrows before they are popped
499-
// from the worklist.
500-
while (true) {
501-
while (!defWorklist.ownedForwards.empty()) {
502-
SILInstruction *ownedForward = defWorklist.ownedForwards.pop_back_val();
503-
// Delete a dead forwarded value before sinking to avoid this pattern:
504-
// %outerVal = destructure_struct %def
505-
// destroy %outerVal <= delete this destroy now
506-
// destroy %def <= so we don't delete this one later
507-
if (deleter.deleteIfDead(ownedForward)) {
508-
LLVM_DEBUG(llvm::dbgs() << " Deleted " << *ownedForward);
509-
continue;
510-
}
511-
// Canonicalize a forwarded owned value before sinking the forwarding
512-
// instruction, and sink the instruction before canonicalizing the owned
513-
// value being forwarded. Process 'ownedForwards' in reverse since
514-
// they may be chained, and CanonicalizeBorrowScopes pushes them
515-
// top-down.
516-
for (auto result : ownedForward->getResults()) {
517-
canonicalizer.canonicalizeValueLifetime(result);
518-
}
519-
if (sinkOwnedForward(ownedForward, postOrderAnalysis, domTree)) {
520-
changed = true;
521-
// Sinking 'ownedForward' may create an opportunity to sink its
522-
// operand. This handles chained forwarding instructions that were
523-
// pushed onto the list out-of-order.
524-
if (SILInstruction *forwardDef =
525-
CanonicalizeOSSALifetime::getCanonicalCopiedDef(
526-
ownedForward->getOperand(0))
527-
->getDefiningInstruction()) {
528-
if (CanonicalizeBorrowScope::isRewritableOSSAForward(forwardDef)) {
529-
defWorklist.ownedForwards.insert(forwardDef);
530-
}
497+
}
498+
// borrowCanonicalizer performs all modifications through deleter's
499+
// callbacks, so we don't need to explicitly check for changes.
500+
CanonicalizeBorrowScope borrowCanonicalizer(deleter);
501+
// The utilities in this loop cannot delete borrows before they are popped
502+
// from the worklist.
503+
while (true) {
504+
while (!defWorklist.ownedForwards.empty()) {
505+
assert(canonicalizeBorrows);
506+
507+
SILInstruction *ownedForward = defWorklist.ownedForwards.pop_back_val();
508+
// Delete a dead forwarded value before sinking to avoid this pattern:
509+
// %outerVal = destructure_struct %def
510+
// destroy %outerVal <= delete this destroy now
511+
// destroy %def <= so we don't delete this one later
512+
if (deleter.deleteIfDead(ownedForward)) {
513+
LLVM_DEBUG(llvm::dbgs() << " Deleted " << *ownedForward);
514+
continue;
515+
}
516+
// Canonicalize a forwarded owned value before sinking the forwarding
517+
// instruction, and sink the instruction before canonicalizing the owned
518+
// value being forwarded. Process 'ownedForwards' in reverse since
519+
// they may be chained, and CanonicalizeBorrowScopes pushes them
520+
// top-down.
521+
for (auto result : ownedForward->getResults()) {
522+
canonicalizer.canonicalizeValueLifetime(result);
523+
}
524+
if (sinkOwnedForward(ownedForward, postOrderAnalysis, domTree)) {
525+
changed = true;
526+
// Sinking 'ownedForward' may create an opportunity to sink its
527+
// operand. This handles chained forwarding instructions that were
528+
// pushed onto the list out-of-order.
529+
if (SILInstruction *forwardDef =
530+
CanonicalizeOSSALifetime::getCanonicalCopiedDef(
531+
ownedForward->getOperand(0))
532+
->getDefiningInstruction()) {
533+
if (CanonicalizeBorrowScope::isRewritableOSSAForward(forwardDef)) {
534+
defWorklist.ownedForwards.insert(forwardDef);
531535
}
532536
}
533537
}
534-
if (defWorklist.borrowedValues.empty())
535-
break;
538+
}
539+
if (defWorklist.borrowedValues.empty())
540+
break;
536541

537-
BorrowedValue borrow(defWorklist.borrowedValues.pop_back_val());
538-
borrowCanonicalizer.canonicalizeBorrowScope(borrow);
539-
for (CopyValueInst *copy : borrowCanonicalizer.getUpdatedCopies()) {
540-
defWorklist.updateForCopy(copy);
541-
}
542-
// Dead borrow scopes must be removed as uses before canonicalizing the
543-
// outer copy.
544-
if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(borrow.value)) {
545-
if (hasOnlyEndOfScopeOrEndOfLifetimeUses(beginBorrow)) {
546-
deleter.recursivelyDeleteUsersIfDead(beginBorrow);
547-
}
542+
BorrowedValue borrow(defWorklist.borrowedValues.pop_back_val());
543+
assert(canonicalizeBorrows || !borrow.isLocalScope());
544+
545+
borrowCanonicalizer.canonicalizeBorrowScope(borrow);
546+
for (CopyValueInst *copy : borrowCanonicalizer.getUpdatedCopies()) {
547+
defWorklist.updateForCopy(copy);
548+
}
549+
// Dead borrow scopes must be removed as uses before canonicalizing the
550+
// outer copy.
551+
if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(borrow.value)) {
552+
if (hasOnlyEndOfScopeOrEndOfLifetimeUses(beginBorrow)) {
553+
deleter.recursivelyDeleteUsersIfDead(beginBorrow);
548554
}
549555
}
550556
deleter.cleanupDeadInstructions();

‎lib/SILOptimizer/Utils/CanonicalizeBorrowScope.cpp

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,8 @@ SILValue CanonicalizeBorrowScope::findDefInBorrowScope(SILValue value) {
209209
///
210210
/// \p innerValue is either the initial begin_borrow, or a forwarding operation
211211
/// within the borrow scope.
212+
///
213+
/// Note: This must always return true when innerValue is a function argument.
212214
template <typename Visitor>
213215
bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
214216
Visitor &visitor) {
@@ -259,10 +261,16 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
259261
case OperandOwnership::ForwardingUnowned:
260262
case OperandOwnership::PointerEscape:
261263
// Pointer escapes are only allowed if they use the guaranteed value,
262-
// which means that the escaped value must be confied to the current
263-
// borrow scope.
264-
if (use->get().getOwnershipKind() != OwnershipKind::Guaranteed)
264+
// which means that the escaped value must be confined to the current
265+
// borrow scope. visitBorrowScopeUses must never return false when
266+
// borrowedValue is a SILFunctionArgument.
267+
if (use->get().getOwnershipKind() != OwnershipKind::Guaranteed
268+
&& !isa<SILFunctionArgument>(borrowedValue.value)) {
269+
return false;
270+
}
271+
if (!visitor.visitUse(use)) {
265272
return false;
273+
}
266274
break;
267275

268276
case OperandOwnership::ForwardingBorrow:
@@ -403,6 +411,8 @@ void CanonicalizeBorrowScope::filterOuterBorrowUseInsts(
403411
namespace {
404412

405413
/// Remove redundant copies/destroys within a borrow scope.
414+
///
415+
/// The visitor callbacks must always return true since this rewrites in-place.
406416
class RewriteInnerBorrowUses {
407417
CanonicalizeBorrowScope &scope;
408418

@@ -472,6 +482,8 @@ class RewriteInnerBorrowUses {
472482
/// visitor. They are rewritten separately.
473483
///
474484
/// Implements visitBorrowScopeUses<Visitor>
485+
///
486+
/// The visitor callbacks must always return true since this rewrites in-place.
475487
class RewriteOuterBorrowUses {
476488
CanonicalizeBorrowScope &scope;
477489

@@ -794,8 +806,9 @@ bool CanonicalizeBorrowScope::canonicalizeFunctionArgument(
794806

795807
RewriteInnerBorrowUses innerRewriter(*this);
796808
beginVisitBorrowScopeUses(); // reset the def/use worklist
809+
797810
bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
798-
assert(succeed && "should be filtered by FindBorrowScopeUses");
811+
assert(succeed && "must always succeed for function arguments");
799812
return true;
800813
}
801814

‎test/SILOptimizer/assemblyvision_remark/basic.swift

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,13 +142,12 @@ public class SubKlass : Klass {
142142
}
143143

144144
func lookThroughCast(x: SubKlass) -> Klass {
145-
return x as Klass // expected-remark {{retain of type 'SubKlass'}}
146-
// expected-note @-2:22 {{of 'x'}}
145+
return x as Klass // expected-remark {{retain of type 'Klass'}}
146+
// expected-note @-2:22 {{of 'x.upcast<$Klass>'}}
147147
}
148148

149149
func lookThroughRefCast(x: Klass) -> SubKlass {
150-
return x as! SubKlass // expected-remark {{retain of type 'Klass'}}
151-
// expected-note @-2:25 {{of 'x'}}
150+
return x as! SubKlass // expected-remark {{retain of type 'SubKlass'}}
152151
}
153152

154153
func lookThroughEnum(x: Klass?) -> Klass {

‎test/SILOptimizer/copy_propagation_borrow.sil

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
// RUN: %target-sil-opt -copy-propagation -canonical-ossa-rewrite-borrows -enable-sil-verify-all %s | %FileCheck %s
1+
// RUN: %target-sil-opt -copy-propagation -canonical-ossa-rewrite-borrows -enable-sil-verify-all -module-name Swift %s | %FileCheck %s
2+
// RUN: %target-sil-opt -copy-propagation -enable-sil-verify-all -module-name Swift %s | %FileCheck %s --check-prefixes=CHECK-NOSCOPES
23
//
34
// Most CopyPropagation tests are still in copy_propagation_opaque.sil.
45
//
@@ -12,8 +13,21 @@ import Builtin
1213

1314
typealias AnyObject = Builtin.AnyObject
1415

16+
protocol Error {}
17+
1518
class B { }
1619

20+
21+
struct UInt64 {
22+
@_hasStorage public var _value: Builtin.Int64 { get set }
23+
init(_value: Builtin.Int64)
24+
}
25+
26+
struct Int64 {
27+
@_hasStorage public var _value: Builtin.Int64 { get set }
28+
init(_value: Builtin.Int64)
29+
}
30+
1731
class C {
1832
var a: Int64
1933
}
@@ -59,16 +73,6 @@ struct HasObjects {
5973
var object1: C
6074
}
6175

62-
struct UInt64 {
63-
@_hasStorage public var _value: Builtin.Int64 { get set }
64-
init(_value: Builtin.Int64)
65-
}
66-
67-
struct Int64 {
68-
@_hasStorage public var _value: Builtin.Int64 { get set }
69-
init(_value: Builtin.Int64)
70-
}
71-
7276
internal struct _StringObject {
7377
@usableFromInline
7478
@_hasStorage internal var _countAndFlagsBits: UInt64 { get set }
@@ -1178,3 +1182,52 @@ bb0(%0 : @guaranteed $String):
11781182
%5 = struct $String.UTF16View (%3 : $_StringGuts)
11791183
return %5 : $String.UTF16View
11801184
}
1185+
1186+
// visitBorrowScopeUses first looks through this copy then sees a
1187+
// unowned forwarding operation (ref_to_unmanaged). It generally
1188+
// cannot prove that all uses of an unowned forward are within the
1189+
// borrow scope. This is still safe, however, because the borrowed
1190+
// value is a function argument. In fact, borrow scope rewriting
1191+
// *must* succeed because it begins rewriting guaranteed function
1192+
// arguments without checking the uses first.
1193+
//
1194+
// CHECK-NOSCOPES-LABEL: sil [ossa] @testEscapingGuaranteedToOwned : $@convention(method) (@guaranteed C) -> () {
1195+
// CHECK-NOSCOPES: bb0(%0 : @guaranteed $C):
1196+
// CHECK-NOSCOPES-NEXT: %1 = ref_to_unmanaged %0 : $C to $@sil_unmanaged C
1197+
// CHECK-NOSCOPES-NEXT: tuple ()
1198+
// CHECK-NOSCOPES-NEXT: return
1199+
// CHECK-NOSCOPES-LABEL: } // end sil function 'testEscapingGuaranteedToOwned'
1200+
sil [ossa] @testEscapingGuaranteedToOwned : $@convention(method) (@guaranteed C) -> () {
1201+
bb0(%0 : @guaranteed $C):
1202+
%1 = copy_value %0 : $C
1203+
%2 = ref_to_unmanaged %1 : $C to $@sil_unmanaged C
1204+
destroy_value %1 : $C
1205+
%7 = tuple ()
1206+
return %7 : $()
1207+
}
1208+
1209+
// visitBorrowScopeUses first looks through this copy, then a normal
1210+
// forwarding operation (convert_function), then an unowned forwarding
1211+
// operation (convert_escape_to_noescape). It must successfully
1212+
// analyze and replace all uses because borrow scope rewriting begins
1213+
// rewriting guaranteed function arguments without checking the uses
1214+
// first.
1215+
//
1216+
// CHECK-NOSCOPES-LABEL: sil [ossa] @testEscapingEscapingCast : $@convention(method) (@guaranteed @callee_guaranteed (Int64) -> Int64) -> () {
1217+
// CHECK-NOSCOPES: bb0(%0 : @guaranteed $@callee_guaranteed (Int64) -> Int64):
1218+
// CHECK-NOSCOPES-NEXT: %1 = convert_function %0 : $@callee_guaranteed (Int64) -> Int64 to $@callee_guaranteed (Int64) -> (Int64, @error Error) // user: %2
1219+
// CHECK-NOSCOPES-NEXT: %2 = convert_escape_to_noescape %1 : $@callee_guaranteed (Int64) -> (Int64, @error Error) to $@noescape @callee_guaranteed (Int64) -> (Int64, @error Error)
1220+
// CHECK-NOSCOPES-NEXT: = tuple ()
1221+
// CHECK-NOSCOPES-NEXT: return
1222+
// CHECK-NOSCOPES-LABEL: } // end sil function 'testEscapingEscapingCast'
1223+
sil [ossa] @testEscapingEscapingCast : $@convention(method) (@guaranteed @callee_guaranteed (Int64) -> Int64) -> () {
1224+
bb0(%0 : @guaranteed $@callee_guaranteed (Int64) -> Int64):
1225+
%1 = copy_value %0 : $@callee_guaranteed (Int64) -> Int64
1226+
%2 = convert_function %1 : $@callee_guaranteed (Int64) -> Int64 to $@callee_guaranteed (Int64) -> (Int64, @error Error)
1227+
%3 = copy_value %2 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
1228+
%4 = convert_escape_to_noescape %3 : $@callee_guaranteed (Int64) -> (Int64, @error Error) to $@noescape @callee_guaranteed (Int64) -> (Int64, @error Error) // user: %29
1229+
destroy_value %3 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
1230+
destroy_value %2 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
1231+
%7 = tuple ()
1232+
return %7 : $()
1233+
}

0 commit comments

Comments
 (0)