Skip to content

Commit c2fd130

Browse files
meg-guptameg-gupta
authored
Merge pull request #74795 from meg-gupta/copyelimexpand
Enable ClosureLifetimeFixup's capture copy elimination for copyable types as well.
2 parents e899ff2 + a470b04 commit c2fd130

6 files changed

+114
-66
lines changed

‎lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

Lines changed: 47 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,12 @@ llvm::cl::opt<bool> DisableConvertEscapeToNoEscapeSwitchEnumPeephole(
4141
"Disable the convert_escape_to_noescape switch enum peephole. "),
4242
llvm::cl::Hidden);
4343

44+
llvm::cl::opt<bool> DisableCopyEliminationOfCopyableCapture(
45+
"sil-disable-copy-elimination-of-copyable-closure-capture",
46+
llvm::cl::init(false),
47+
llvm::cl::desc("Don't eliminate copy_addr of Copyable closure captures "
48+
"inserted by SILGen"));
49+
4450
using namespace swift;
4551

4652
/// Given an optional diamond, return the bottom of the diamond.
@@ -734,7 +740,7 @@ static SILValue tryRewriteToPartialApplyStack(
734740
SmallVector<SILInstruction *, 4> lifetimeEnds;
735741
collectStackClosureLifetimeEnds(lifetimeEnds, closureOp);
736742

737-
// For noncopyable address-only captures, see if we can eliminate the copy
743+
// For address-only captures, see if we can eliminate the copy
738744
// that SILGen emitted to allow the original partial_apply to take ownership.
739745
// We do this here because otherwise the move checker will see the copy as an
740746
// attempt to consume the value, which we don't want.
@@ -760,14 +766,14 @@ static SILValue tryRewriteToPartialApplyStack(
760766
LLVM_DEBUG(llvm::dbgs() << "-- not an alloc_stack\n");
761767
continue;
762768
}
763-
764-
// This would be a nice optimization to attempt for all types, but for now,
765-
// limit the effect to move-only types.
766-
if (!copy->getType().isMoveOnly()) {
767-
LLVM_DEBUG(llvm::dbgs() << "-- not move-only\n");
768-
continue;
769+
770+
if (DisableCopyEliminationOfCopyableCapture) {
771+
if (!copy->getType().isMoveOnly()) {
772+
LLVM_DEBUG(llvm::dbgs() << "-- not move-only\n");
773+
continue;
774+
}
769775
}
770-
776+
771777
// Is the capture a borrow?
772778

773779
auto paramIndex = i + appliedArgStartIdx;
@@ -806,33 +812,33 @@ static SILValue tryRewriteToPartialApplyStack(
806812
if (!lookThroughMarkDependenceChainForValue(mark, newPA) ||
807813
mark->getBase() != stack) {
808814
LLVM_DEBUG(llvm::dbgs() << "-- had unexpected mark_dependence use\n";
809-
use->getUser()->print(llvm::dbgs());
810-
llvm::dbgs() << "\n");
815+
use->getUser()->print(llvm::dbgs()); llvm::dbgs() << "\n");
811816
initialization = nullptr;
812817
break;
813818
}
814819
markDep = mark;
815820

816821
continue;
817822
}
818-
823+
819824
// If we saw more than just the initialization, this isn't a pattern we
820825
// recognize.
821826
if (initialization) {
822-
LLVM_DEBUG(llvm::dbgs() << "-- had non-initialization, non-partial-apply use\n";
823-
use->getUser()->print(llvm::dbgs());
824-
llvm::dbgs() << "\n");
825-
827+
LLVM_DEBUG(llvm::dbgs()
828+
<< "-- had non-initialization, non-partial-apply use\n";
829+
use->getUser()->print(llvm::dbgs()); llvm::dbgs() << "\n");
830+
826831
initialization = nullptr;
827832
break;
828833
}
829834
if (auto possibleInit = dyn_cast<CopyAddrInst>(use->getUser())) {
830835
// Should copy the source and initialize the destination.
831-
if (possibleInit->isTakeOfSrc()
832-
|| !possibleInit->isInitializationOfDest()) {
833-
LLVM_DEBUG(llvm::dbgs() << "-- had non-initialization, non-partial-apply use\n";
834-
use->getUser()->print(llvm::dbgs());
835-
llvm::dbgs() << "\n");
836+
if (possibleInit->isTakeOfSrc() ||
837+
!possibleInit->isInitializationOfDest()) {
838+
LLVM_DEBUG(
839+
llvm::dbgs()
840+
<< "-- had non-initialization, non-partial-apply use\n";
841+
use->getUser()->print(llvm::dbgs()); llvm::dbgs() << "\n");
836842

837843
break;
838844
}
@@ -860,33 +866,36 @@ static SILValue tryRewriteToPartialApplyStack(
860866
LLVM_DEBUG(llvm::dbgs() << "++ found original:\n";
861867
orig->print(llvm::dbgs());
862868
llvm::dbgs() << "\n");
863-
864-
bool origIsUnusedDuringClosureLifetime = true;
865869

866-
class OrigUnusedDuringClosureLifetimeWalker
870+
bool origIsUnmodifiedDuringClosureLifetime = true;
871+
872+
class OrigUnmodifiedDuringClosureLifetimeWalker
867873
: public TransitiveAddressWalker<
868-
OrigUnusedDuringClosureLifetimeWalker> {
874+
OrigUnmodifiedDuringClosureLifetimeWalker> {
869875
SSAPrunedLiveness &closureLiveness;
870-
bool &origIsUnusedDuringClosureLifetime;
876+
bool &origIsUnmodifiedDuringClosureLifetime;
877+
871878
public:
872-
OrigUnusedDuringClosureLifetimeWalker(SSAPrunedLiveness &closureLiveness,
873-
bool &origIsUnusedDuringClosureLifetime)
874-
: closureLiveness(closureLiveness),
875-
origIsUnusedDuringClosureLifetime(origIsUnusedDuringClosureLifetime)
876-
{}
879+
OrigUnmodifiedDuringClosureLifetimeWalker(
880+
SSAPrunedLiveness &closureLiveness,
881+
bool &origIsUnmodifiedDuringClosureLifetime)
882+
: closureLiveness(closureLiveness),
883+
origIsUnmodifiedDuringClosureLifetime(
884+
origIsUnmodifiedDuringClosureLifetime) {}
877885

878886
bool visitUse(Operand *origUse) {
879887
LLVM_DEBUG(llvm::dbgs() << "looking at use\n";
880888
origUse->getUser()->printInContext(llvm::dbgs());
881889
llvm::dbgs() << "\n");
882-
890+
883891
// If the user doesn't write to memory, then it's harmless.
884892
if (!origUse->getUser()->mayWriteToMemory()) {
885893
return true;
886894
}
887895
if (closureLiveness.isWithinBoundary(origUse->getUser())) {
888-
origIsUnusedDuringClosureLifetime = false;
889-
LLVM_DEBUG(llvm::dbgs() << "-- original has other possibly writing use during closure lifetime\n";
896+
origIsUnmodifiedDuringClosureLifetime = false;
897+
LLVM_DEBUG(llvm::dbgs() << "-- original has other possibly writing "
898+
"use during closure lifetime\n";
890899
origUse->getUser()->print(llvm::dbgs());
891900
llvm::dbgs() << "\n");
892901
return false;
@@ -895,12 +904,12 @@ static SILValue tryRewriteToPartialApplyStack(
895904
}
896905
};
897906

898-
OrigUnusedDuringClosureLifetimeWalker origUseWalker(closureLiveness,
899-
origIsUnusedDuringClosureLifetime);
907+
OrigUnmodifiedDuringClosureLifetimeWalker origUseWalker(
908+
closureLiveness, origIsUnmodifiedDuringClosureLifetime);
900909
auto walkResult = std::move(origUseWalker).walk(orig);
901-
902-
if (walkResult == AddressUseKind::Unknown
903-
|| !origIsUnusedDuringClosureLifetime) {
910+
911+
if (walkResult == AddressUseKind::Unknown ||
912+
!origIsUnmodifiedDuringClosureLifetime) {
904913
continue;
905914
}
906915

‎test/SILOptimizer/closure-lifetime-fixup-debuginfo.sil

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-sil-opt -enable-sil-verify-all -closure-lifetime-fixup -emit-verbose-sil %s | %FileCheck %s
1+
// RUN: %target-sil-opt -enable-sil-verify-all -closure-lifetime-fixup -emit-verbose-sil -sil-disable-copy-elimination-of-copyable-closure-capture=true %s | %FileCheck %s
22

33
import Builtin
44
import Swift

‎test/SILOptimizer/closure-lifetime-fixup.sil

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -267,22 +267,16 @@ bb3:
267267

268268
// CHECK-LABEL: sil [ossa] @test_alloc_stack_arg_with_frontier_outside_try_apply_successors : $@convention(thin) <Self> (@in_guaranteed Self) -> @error any Error {
269269
// CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] :
270-
// CHECK: [[STACK:%[^,]+]] = alloc_stack $Self
271270
// CHECK: try_apply undef() {{.*}}, normal [[SUCCESS_1:bb[0-9]+]], error [[FAILURE_1:bb[0-9]+]]
272271
// CHECK: [[SUCCESS_1]]
273-
// CHECK: copy_addr [[INSTANCE]] to [init] [[STACK]] : $*Self, {{.*}} scope [[STACK_SCOPE:[0-9]+]]
274-
// CHECK: [[CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] undef<Self>([[STACK]])
275-
// CHECK: [[DEPENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[CLOSURE]] {{.*}} on [[STACK]]
272+
// CHECK: [[CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] undef<Self>([[INSTANCE]])
273+
// CHECK: [[DEPENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[CLOSURE]] {{.*}} on [[INSTANCE]]
276274
// CHECK: try_apply undef([[DEPENDENCY]]) {{.*}}, normal [[SUCCESS_2:bb[0-9]+]], error [[FAILURE_2:bb[0-9]+]]
277275
// CHECK: [[SUCCESS_2]]
278276
// CHECK: destroy_value [[DEPENDENCY]]
279-
// CHECK: destroy_addr [[STACK]]
280-
// CHECK: dealloc_stack [[STACK]] : $*Self, {{.*}} scope [[STACK_SCOPE]]
281277
// CHECK: [[FAILURE_1]]
282-
// CHECK: dealloc_stack [[STACK]] : $*Self, {{.*}} scope [[STACK_SCOPE]]
283278
// CHECK: throw
284279
// CHECK: [[FAILURE_2]]
285-
// CHECK-NOT: dealloc_stack
286280
// CHECK: unreachable
287281
// CHECK-LABEL: } // end sil function 'test_alloc_stack_arg_with_frontier_outside_try_apply_successors'
288282
sil [ossa] @test_alloc_stack_arg_with_frontier_outside_try_apply_successors : $@convention(thin) <Self> (@in_guaranteed Self) -> (@error any Error) {

‎test/SILOptimizer/closure_lifetime_fixup_copyelim.swift

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// RUN: %target-swift-frontend -emit-sil %s -O| %FileCheck %s
2+
3+
public enum Foo {
4+
case bar
5+
case baz(Int)
6+
}
7+
8+
public protocol Frob {
9+
func foo(_ x: Int) -> Foo
10+
}
11+
12+
public struct Nicate {
13+
public var frob: any Frob
14+
public var isInitializing: Bool
15+
16+
// CHECK-LABEL: sil @$s31closure_lifetime_fixup_copyelim6NicateV4slowyS2iF :
17+
// CHECK-NOT: copy_addr
18+
// CHECK-LABEL: } // end sil function '$s31closure_lifetime_fixup_copyelim6NicateV4slowyS2iF'
19+
public func slow(_ x: Int) -> Int {
20+
let foo = frob.foo(x)
21+
switch foo {
22+
case .bar:
23+
return 10
24+
case .baz(let y):
25+
if y == 0 && isInitializing {
26+
return foos[x]
27+
} else {
28+
return y
29+
}
30+
}
31+
}
32+
}
33+
34+
private let foos = [1, 2, 3]

‎test/SILOptimizer/moveonly_closure_lifetime_fixup_address_only_borrowed_captures.swift

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ struct E<T>: ~Copyable {
88
var t: T
99
}
1010

11+
class Klass {}
1112
struct C<T> {
1213
var t: T
1314
}
@@ -19,15 +20,31 @@ func escaping(_ x: @escaping () -> ()) { escaper = x }
1920

2021
func borrow<T>(_: borrowing E<T>) {}
2122
func borrow<T>(_: borrowing C<T>) {}
23+
func mutate<T>(_: inout C<T>) {}
24+
25+
// CHECK-LABEL: sil {{.*}}14testCopySafetyyyAA1CVyxGlF :
26+
// CHECK: [[STK1:%.*]] = alloc_stack {{.*}} $C<T>
27+
// CHECK: copy_addr %0 to [init] [[STK1]]
28+
// CHECK: partial_apply {{.*}}[on_stack] {{.*}}([[STK1]]) : $@convention(thin) <τ_0_0> (@inout_aliasable C<τ_0_0>) -> ()
29+
// CHECK: [[STK2:%.*]] = alloc_stack {{.*}} $C<T>
30+
// CHECK: [[BA:%.*]] = begin_access [read] [static] [[STK1]]
31+
// CHECK: copy_addr [[BA]] to [init] [[STK2]]
32+
// CHECK: end_access [[BA]]
33+
// CHECK: partial_apply {{.*}}[on_stack] {{.*}}([[STK2]]) : $@convention(thin) <τ_0_0> (@in_guaranteed C<τ_0_0>) -> ()
34+
func testCopySafety<T>(_ c: C<T>) {
35+
var mutC = c
36+
nonescaping({ mutate(&mutC) }, and: {[mutC] in borrow(mutC) })
37+
}
2238

23-
func testMultiCapture<T>(_ e: borrowing E<T>, _ c: C<T>) {
24-
// CHECK: [[C_STK:%.*]] = alloc_stack $C<T>
25-
// CHECK: copy_addr %1 to [init] [[C_STK]] : $*C<T>
26-
// CHECK: partial_apply {{.*}}[on_stack] {{.*}}([[C_STK]], %0) :
27-
nonescaping {
28-
borrow(c)
29-
borrow(e)
30-
}
39+
// CHECK-LABEL: sil {{.*}}18testVarReadCaptureyyAA1CVyxGlF :
40+
// CHECK: [[STK:%.*]] = alloc_stack {{.*}} $C<T>
41+
// CHECK: copy_addr %0 to [init] [[STK]]
42+
// CHECK: partial_apply {{.*}}[on_stack] {{.*}}([[STK]]) : $@convention(thin) <τ_0_0> (@inout_aliasable C<τ_0_0>) -> ()
43+
func testVarReadCapture<T>(_ c: C<T>) {
44+
var mutC = c
45+
nonescaping {
46+
borrow(mutC)
47+
}
3148
}
3249

3350
// CHECK-LABEL: sil {{.*}}16testNeverEscaped

‎test/SILOptimizer/opaque_values_Onone.swift

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,10 @@ func doit<T>(_ f: () -> T) -> T {
6868
}
6969
// CHECK-LABEL: sil hidden @duplicate1 : {{.*}} {
7070
// CHECK: {{bb[0-9]+}}({{%[^,]+}} : $*Value, {{%[^,]+}} : $*Value, [[INSTANCE_ADDR_IN:%[^,]+]] : $*Value):
71-
// CHECK: [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $Value
7271
// CHECK: [[OUTPUT_TUPLE_ADDR:%[^,]+]] = alloc_stack $(Value, Value)
7372
// CHECK: [[DUPLICATE_CLOSURE:%[^,]+]] = function_ref @$s19opaque_values_Onone10duplicate15valuex_xtx_tlFx_xtyXEfU_
74-
// CHECK: copy_addr [[INSTANCE_ADDR_IN]] to [init] [[INSTANCE_ADDR]]
75-
// CHECK: [[DUPLICATE_INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[DUPLICATE_CLOSURE]]<Value>([[INSTANCE_ADDR]])
76-
// CHECK: [[DEPENDENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[DUPLICATE_INSTANCE_CLOSURE]] : $@noescape @callee_guaranteed () -> @out (Value, Value) on [[INSTANCE_ADDR]] : $*Value
73+
// CHECK: [[DUPLICATE_INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[DUPLICATE_CLOSURE]]<Value>([[INSTANCE_ADDR_IN]])
74+
// CHECK: [[DEPENDENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[DUPLICATE_INSTANCE_CLOSURE]] : $@noescape @callee_guaranteed () -> @out (Value, Value) on [[INSTANCE_ADDR_IN]] : $*Value
7775
// CHECK: [[CONVERTED:%[^,]+]] = convert_function [[DEPENDENDENCY]]
7876
// CHECK: apply {{%[^,]+}}<(Value, Value)>([[OUTPUT_TUPLE_ADDR]], [[CONVERTED]])
7977
// CHECK-LABEL: } // end sil function 'duplicate1'
@@ -92,12 +90,10 @@ func duplicate1<Value>(value: Value) -> (Value, Value) {
9290
}
9391
// CHECK-LABEL: sil hidden @duplicate2 : {{.*}} {
9492
// CHECK: {{bb[0-9]+}}({{%[^,]+}} : $*Value, {{%[^,]+}} : $*Value, [[INSTANCE_ADDR_IN:%[^,]+]] : $*Value):
95-
// CHECK: [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $Value
9693
// CHECK: [[OUTPUT_TUPLE_ADDR:%[^,]+]] = alloc_stack $(one: Value, two: Value)
9794
// CHECK: [[DUPLICATE_CLOSURE:%[^,]+]] = function_ref @$s19opaque_values_Onone10duplicate25valuex3one_x3twotx_tlFxAD_xAEtyXEfU_
98-
// CHECK: copy_addr [[INSTANCE_ADDR_IN]] to [init] [[INSTANCE_ADDR]]
99-
// CHECK: [[DUPLICATE_INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[DUPLICATE_CLOSURE]]<Value>([[INSTANCE_ADDR]])
100-
// CHECK: [[DEPENDENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[DUPLICATE_INSTANCE_CLOSURE]] : $@noescape @callee_guaranteed () -> @out (one: Value, two: Value) on [[INSTANCE_ADDR]] : $*Value
95+
// CHECK: [[DUPLICATE_INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[DUPLICATE_CLOSURE]]<Value>([[INSTANCE_ADDR_IN]])
96+
// CHECK: [[DEPENDENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[DUPLICATE_INSTANCE_CLOSURE]] : $@noescape @callee_guaranteed () -> @out (one: Value, two: Value) on [[INSTANCE_ADDR_IN]] : $*Value
10197
// CHECK: [[CONVERTED:%[^,]+]] = convert_function [[DEPENDENDENCY]]
10298
// CHECK: apply {{%[^,]+}}<(one: Value, two: Value)>([[OUTPUT_TUPLE_ADDR]], [[CONVERTED]])
10399
// CHECK-LABEL: } // end sil function 'duplicate2'
@@ -129,10 +125,8 @@ func duplicate_with_int2<Value>(value: Value) -> ((Value, Value), Int) {
129125

130126
// CHECK-LABEL: sil hidden @duplicate_with_int3 : {{.*}} {
131127
// CHECK: {{bb[0-9]+}}({{%[^,]+}} : $*Value, {{%[^,]+}} : $*Value, {{%[^,]+}} : $*Value, {{%[^,]+}} : $*Value, [[INSTANCE_ADDR_IN:%[^,]+]] : $*Value):
132-
// CHECK: [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $Value
133128
// CHECK: [[CLOSURE:%[^,]+]] = function_ref @$s19opaque_values_Onone19duplicate_with_int35valueSi_x_x_x_SitxttSitx_tlFSi_x_x_x_SitxttSityXEfU_
134-
// CHECK: copy_addr [[INSTANCE_ADDR_IN]] to [init] [[INSTANCE_ADDR]]
135-
// CHECK: [[INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[CLOSURE]]<Value>([[INSTANCE_ADDR]])
129+
// CHECK: [[INSTANCE_CLOSURE:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[CLOSURE]]<Value>([[INSTANCE_ADDR_IN]])
136130
// CHECK: [[DEPENDENCY:%[^,]+]] = mark_dependence [nonescaping] [[INSTANCE_CLOSURE]]
137131
// CHECK: [[CONVERTED:%[^,]+]] = convert_function [[DEPENDENCY]]
138132
// CHECK: apply {{%[^,]+}}<(Int, (Value, (Value, (Value, Int), Value)), Int)>({{%[^,]+}}, [[CONVERTED]])

0 commit comments

Comments
 (0)