Skip to content

Commit 202de3a

Browse files
nate-chandlernate-chandler
authored
Merge pull request #78574 from nate-chandler/cherrypick/release/6.1/rdar142520491
6.1: [OSSACanonicalizeGuaranteed] Don't rewrite consuming uses of move-only values.
2 parents b579a9e + e3ff19e commit 202de3a

File tree

2 files changed

+127
-36
lines changed

2 files changed

+127
-36
lines changed

‎lib/SILOptimizer/Utils/CanonicalizeBorrowScope.cpp‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -96,29 +96,29 @@ bool CanonicalizeBorrowScope::isRewritableOSSAForward(SILInstruction *inst) {
9696
if (inst->getNumOperands() != 1)
9797
return false;
9898

99-
if (isa<OwnershipForwardingSingleValueInstruction>(inst) ||
100-
isa<OwnershipForwardingMultipleValueInstruction>(inst)) {
101-
Operand *forwardedOper = &inst->getOperandRef(0);
102-
// Trivial conversions do not need to be hoisted out of a borrow scope.
103-
auto operOwnership = forwardedOper->getOperandOwnership();
104-
if (operOwnership == OperandOwnership::TrivialUse)
105-
return false;
106-
// Don't mess with unowned conversions. They need to be copied immediately.
107-
if (operOwnership != OperandOwnership::GuaranteedForwarding &&
108-
operOwnership != OperandOwnership::ForwardingConsume) {
109-
return false;
110-
}
111-
assert(operOwnership == OperandOwnership::GuaranteedForwarding ||
112-
operOwnership == OperandOwnership::ForwardingConsume);
113-
114-
// Filter instructions that belong to a Forwarding*ValueInst mixin but
115-
// cannot be converted to forward owned value (struct_extract).
116-
if (!canOpcodeForwardOwnedValues(forwardedOper))
117-
return false;
99+
if (!isa<OwnershipForwardingSingleValueInstruction>(inst) &&
100+
!isa<OwnershipForwardingMultipleValueInstruction>(inst))
101+
return false;
118102

119-
return true;
103+
Operand *forwardedOper = &inst->getOperandRef(0);
104+
// Trivial conversions do not need to be hoisted out of a borrow scope.
105+
auto operOwnership = forwardedOper->getOperandOwnership();
106+
if (operOwnership == OperandOwnership::TrivialUse)
107+
return false;
108+
// Don't mess with unowned conversions. They need to be copied immediately.
109+
if (operOwnership != OperandOwnership::GuaranteedForwarding &&
110+
operOwnership != OperandOwnership::ForwardingConsume) {
111+
return false;
120112
}
121-
return false;
113+
assert(operOwnership == OperandOwnership::GuaranteedForwarding ||
114+
operOwnership == OperandOwnership::ForwardingConsume);
115+
116+
// Filter instructions that belong to a Forwarding*ValueInst mixin but
117+
// cannot be converted to forward owned value (struct_extract).
118+
if (!canOpcodeForwardOwnedValues(forwardedOper))
119+
return false;
120+
121+
return true;
122122
}
123123

124124
/// Return the root of a borrowed extended lifetime for \p def or invalid.
@@ -218,8 +218,6 @@ SILValue CanonicalizeBorrowScope::findDefInBorrowScope(SILValue value) {
218218
///
219219
/// \p innerValue is either the initial begin_borrow, or a forwarding operation
220220
/// within the borrow scope.
221-
///
222-
/// Note: This must always return true when innerValue is a function argument.
223221
template <typename Visitor>
224222
bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
225223
Visitor &visitor) {
@@ -277,14 +275,12 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
277275
case OperandOwnership::PointerEscape:
278276
// Pointer escapes are only allowed if they use the guaranteed value,
279277
// which means that the escaped value must be confined to the current
280-
// borrow scope. visitBorrowScopeUses must never return false when
281-
// borrowedValue is a SILFunctionArgument.
278+
// borrow scope.
282279
if (use->get()->getOwnershipKind() != OwnershipKind::Guaranteed &&
283280
!isa<SILFunctionArgument>(borrowedValue.value)) {
284281
return false;
285282
}
286283
if (!visitor.visitUse(use)) {
287-
assert(!isa<SILFunctionArgument>(borrowedValue.value));
288284
return false;
289285
}
290286
break;
@@ -293,7 +289,6 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
293289
case OperandOwnership::ForwardingConsume:
294290
if (CanonicalizeBorrowScope::isRewritableOSSAForward(user)) {
295291
if (!visitor.visitForwardingUse(use)) {
296-
assert(!isa<SILFunctionArgument>(borrowedValue.value));
297292
return false;
298293
}
299294
break;
@@ -306,7 +301,6 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
306301
case OperandOwnership::BitwiseEscape:
307302
case OperandOwnership::DestroyingConsume:
308303
if (!visitor.visitUse(use)) {
309-
assert(!isa<SILFunctionArgument>(borrowedValue.value));
310304
return false;
311305
}
312306
break;
@@ -394,7 +388,7 @@ class FindBorrowScopeUses {
394388

395389
} // namespace
396390

397-
/// Erase users from \p outerUseInsts that are not within the borrow scope.
391+
/// Erase users from \p outerUseInsts that are actually within the borrow scope.
398392
void CanonicalizeBorrowScope::filterOuterBorrowUseInsts(
399393
OuterUsers &outerUseInsts) {
400394
auto *beginBorrow = cast<BeginBorrowInst>(borrowedValue.value);
@@ -451,6 +445,10 @@ class RewriteInnerBorrowUses {
451445
}
452446
SILValue def = scope.findDefInBorrowScope(value);
453447
if (use->isConsuming()) {
448+
if (use->get()->getType().isMoveOnly()) {
449+
// Can't produce a copy of a non-copyable value on demand. Bail out.
450+
return false;
451+
}
454452
// All in-scope consuming uses need a unique copy in the same block.
455453
auto *copy = dyn_cast<CopyValueInst>(use->get());
456454
if (copy && copy->hasOneUse() && copy->getParent() == user->getParent()) {
@@ -480,7 +478,9 @@ class RewriteInnerBorrowUses {
480478
if (!hasValueOwnership(result)) {
481479
continue;
482480
}
483-
scope.visitBorrowScopeUses(result, *this);
481+
if (!scope.visitBorrowScopeUses(result, *this)) {
482+
return false;
483+
}
484484
}
485485
// Update this operand bypassing any copies.
486486
SILValue value = use->get();
@@ -796,9 +796,7 @@ bool CanonicalizeBorrowScope::consolidateBorrowScope() {
796796
if (outerUseInsts.empty()) {
797797
RewriteInnerBorrowUses innerRewriter(*this);
798798
beginVisitBorrowScopeUses(); // reset the def/use worklist
799-
bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
800-
assert(succeed && "should be filtered by FindBorrowScopeUses");
801-
return true;
799+
return visitBorrowScopeUses(borrowedValue.value, innerRewriter);
802800
}
803801
LLVM_DEBUG(llvm::dbgs() << " Outer uses:\n";
804802
for (SILInstruction *inst
@@ -826,11 +824,25 @@ bool CanonicalizeBorrowScope::canonicalizeFunctionArgument(
826824
RewriteInnerBorrowUses innerRewriter(*this);
827825
beginVisitBorrowScopeUses(); // reset the def/use worklist
828826

829-
bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
830-
assert(succeed && "must always succeed for function arguments");
831-
return true;
827+
return visitBorrowScopeUses(borrowedValue.value, innerRewriter);
832828
}
833829

830+
namespace swift::test {
831+
// Arguments:
832+
// - SILFunctionArgument: function argument to canonicalize
833+
// Dumps:
834+
// - function after argument canonicalization
835+
static FunctionTest CanonicalizeFunctionArgumentTest(
836+
"canonicalize_function_argument",
837+
[](auto &function, auto &arguments, auto &test) {
838+
auto *argument = cast<SILFunctionArgument>(arguments.takeBlockArgument());
839+
InstructionDeleter deleter;
840+
CanonicalizeBorrowScope canonicalizer(&function, deleter);
841+
canonicalizer.canonicalizeFunctionArgument(argument);
842+
function.print(llvm::outs());
843+
});
844+
} // end namespace swift::test
845+
834846
/// Canonicalize a worklist of extended lifetimes. This iterates after rewriting
835847
/// borrow scopes to handle new outer copies and new owned lifetimes from
836848
/// forwarding operations.

‎test/SILOptimizer/canonicalize_function_argument_unit.sil‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// RUN: %target-sil-opt \
2+
// RUN: -test-runner \
3+
// RUN: -module-name Swift \
4+
// RUN: %s \
5+
// RUN: -o /dev/null \
6+
// RUN: 2>&1 | %FileCheck %s
7+
8+
import Builtin
9+
10+
@_marker protocol Copyable {}
11+
12+
class C {}
13+
struct NC : ~Copyable {
14+
var c: C
15+
}
16+
17+
// CHECK-LABEL: begin running test {{.*}} on consume_move_only
18+
// CHECK-LABEL: sil [ossa] @consume_move_only : {{.*}} {
19+
// CHECK: bb0([[C:%[^,]+]] :
20+
// Necessary copy.
21+
// CHECK: [[COPY:%[^,]+]] = copy_value [[C]]
22+
// CHECK: [[NC:%[^,]+]] = struct $NC ([[COPY]]
23+
// CHECK: apply undef([[NC]])
24+
// CHECK-LABEL: } // end sil function 'consume_move_only'
25+
// CHECK-LABEL: end running test {{.*}} on consume_move_only
26+
sil [ossa] @consume_move_only : $@convention(thin) (@guaranteed C) -> () {
27+
bb0(%c : @guaranteed $C):
28+
specify_test "canonicalize_function_argument @argument"
29+
%copy = copy_value %c : $C
30+
%nc = struct $NC (%copy : $C)
31+
apply undef(%nc) : $@convention(thin) (@owned NC) -> ()
32+
%retval = tuple ()
33+
return %retval : $()
34+
}
35+
36+
// CHECK-LABEL: begin running test {{.*}} on borrow_move_only
37+
// CHECK-LABEL: sil [ossa] @borrow_move_only : {{.*}} {
38+
// CHECK: bb0([[C:%[^,]+]] :
39+
// No copy!
40+
// CHECK: [[NC:%[^,]+]] = struct $NC ([[C]]
41+
// CHECK: apply undef([[NC]])
42+
// CHECK-LABEL: } // end sil function 'borrow_move_only'
43+
// CHECK-LABEL: end running test {{.*}} on borrow_move_only
44+
sil [ossa] @borrow_move_only : $@convention(thin) (@guaranteed C) -> () {
45+
bb0(%c : @guaranteed $C):
46+
specify_test "canonicalize_function_argument @argument"
47+
%copy = copy_value %c : $C
48+
%nc = struct $NC (%copy : $C)
49+
apply undef(%nc) : $@convention(thin) (@guaranteed NC) -> ()
50+
destroy_value %nc : $NC
51+
%retval = tuple ()
52+
return %retval : $()
53+
}
54+
55+
// CHECK-LABEL: begin running test {{.*}} on consume_and_borrow_move_only
56+
// CHECK-LABEL: sil [ossa] @consume_and_borrow_move_only : {{.*}} {
57+
// CHECK: bb0([[C:%[^,]+]] :
58+
// No copy!
59+
// CHECK: [[NC1:%[^,]+]] = struct $NC ([[C]]
60+
// CHECK: apply undef([[NC1]])
61+
// Necessary copy.
62+
// CHECK: [[COPY2:%[^,]+]] = copy_value [[C]]
63+
// CHECK: [[NC2:%[^,]+]] = struct $NC ([[COPY2]]
64+
// CHECK: apply undef([[NC2]])
65+
// CHECK-LABEL: } // end sil function 'consume_and_borrow_move_only'
66+
// CHECK-LABEL: end running test {{.*}} on consume_and_borrow_move_only
67+
sil [ossa] @consume_and_borrow_move_only : $@convention(thin) (@guaranteed C) -> () {
68+
bb0(%c : @guaranteed $C):
69+
specify_test "canonicalize_function_argument @argument"
70+
%copy1 = copy_value %c : $C
71+
%nc1 = struct $NC (%copy1 : $C)
72+
apply undef(%nc1) : $@convention(thin) (@guaranteed NC) -> ()
73+
destroy_value %nc1 : $NC
74+
%copy2 = copy_value %c : $C
75+
%nc2 = struct $NC (%copy2 : $C)
76+
apply undef(%nc2) : $@convention(thin) (@owned NC) -> ()
77+
%retval = tuple ()
78+
return %retval : $()
79+
}

0 commit comments

Comments
 (0)