Skip to content

Commit 34a5f03

Browse files
authored
Merge pull request #10806 from gottesmm/swift-4.0-branchrdar32887993
[4.0] [sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values.
2 parents c462d32 + 5af7650 commit 34a5f03

File tree

4 files changed

+147
-60
lines changed

4 files changed

+147
-60
lines changed

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,12 @@
3232
using namespace swift;
3333
using namespace swift::PatternMatch;
3434

35+
/// This flag is used to disable alloc stack optimizations to ease testing of
36+
/// other SILCombine optimizations.
37+
static llvm::cl::opt<bool>
38+
DisableAllocStackOpts("sil-combine-disable-alloc-stack-opts",
39+
llvm::cl::init(false));
40+
3541
SILInstruction*
3642
SILCombiner::visitAllocExistentialBoxInst(AllocExistentialBoxInst *AEBI) {
3743

@@ -334,6 +340,11 @@ struct AllocStackAnalyzer : SILInstructionVisitor<AllocStackAnalyzer> {
334340
} // end anonymous namespace
335341

336342
SILInstruction *SILCombiner::visitAllocStackInst(AllocStackInst *AS) {
343+
// If we are testing SILCombine and we are asked not to eliminate
344+
// alloc_stacks, just return.
345+
if (DisableAllocStackOpts)
346+
return nullptr;
347+
337348
AllocStackAnalyzer Analyzer(AS);
338349
Analyzer.analyze();
339350

lib/SILOptimizer/Utils/Local.cpp

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -891,6 +891,13 @@ static bool useDoesNotKeepClosureAlive(const SILInstruction *I) {
891891
}
892892
}
893893

894+
// *HEY YOU, YES YOU, PLEASE READ*. Even though a textual partial apply is
895+
// printed with the convention of the closed over function upon it, all
896+
// non-inout arguments to a partial_apply are passed at +1. This includes
897+
// arguments that will eventually be passed as guaranteed or in_guaranteed to
898+
// the closed over function. This is because the partial apply is building up a
899+
// boxed aggregate to send off to the closed over function. Of course when you
900+
// call the function, the proper conventions will be used.
894901
void swift::releasePartialApplyCapturedArg(SILBuilder &Builder, SILLocation Loc,
895902
SILValue Arg, SILParameterInfo PInfo,
896903
InstModCallbacks Callbacks) {
@@ -901,51 +908,51 @@ void swift::releasePartialApplyCapturedArg(SILBuilder &Builder, SILLocation Loc,
901908
if (PInfo.isIndirectMutating())
902909
return;
903910

904-
if (isa<AllocStackInst>(Arg)) {
905-
return;
906-
}
907-
908911
// If we have a trivial type, we do not need to put in any extra releases.
909912
if (Arg->getType().isTrivial(Builder.getModule()))
910913
return;
911914

912-
// Otherwise, we need to destroy the argument.
913-
if (Arg->getType().isObject()) {
914-
// If we have qualified ownership, we should just emit a destroy value.
915-
if (Arg->getFunction()->hasQualifiedOwnership()) {
916-
Callbacks.CreatedNewInst(Builder.createDestroyValue(Loc, Arg));
917-
return;
918-
}
919-
920-
if (Arg->getType().hasReferenceSemantics()) {
921-
auto U = Builder.emitStrongRelease(Loc, Arg);
922-
if (U.isNull())
923-
return;
915+
// Otherwise, we need to destroy the argument. If we have an address, just
916+
// emit a destroy_addr.
917+
if (Arg->getType().isAddress()) {
918+
SILInstruction *NewInst = Builder.emitDestroyAddrAndFold(Loc, Arg);
919+
Callbacks.CreatedNewInst(NewInst);
920+
return;
921+
}
924922

925-
if (auto *SRI = U.dyn_cast<StrongRetainInst *>()) {
926-
Callbacks.DeleteInst(SRI);
927-
return;
928-
}
923+
// Otherwise, we have an object. We emit the most optimized form of release
924+
// possible for that value.
929925

930-
Callbacks.CreatedNewInst(U.get<StrongReleaseInst *>());
931-
return;
932-
}
926+
// If we have qualified ownership, we should just emit a destroy value.
927+
if (Arg->getFunction()->hasQualifiedOwnership()) {
928+
Callbacks.CreatedNewInst(Builder.createDestroyValue(Loc, Arg));
929+
return;
930+
}
933931

934-
auto U = Builder.emitReleaseValue(Loc, Arg);
932+
if (Arg->getType().hasReferenceSemantics()) {
933+
auto U = Builder.emitStrongRelease(Loc, Arg);
935934
if (U.isNull())
936935
return;
937936

938-
if (auto *RVI = U.dyn_cast<RetainValueInst *>()) {
939-
Callbacks.DeleteInst(RVI);
937+
if (auto *SRI = U.dyn_cast<StrongRetainInst *>()) {
938+
Callbacks.DeleteInst(SRI);
940939
return;
941940
}
942941

943-
Callbacks.CreatedNewInst(U.get<ReleaseValueInst *>());
942+
Callbacks.CreatedNewInst(U.get<StrongReleaseInst *>());
943+
return;
944+
}
945+
946+
auto U = Builder.emitReleaseValue(Loc, Arg);
947+
if (U.isNull())
948+
return;
949+
950+
if (auto *RVI = U.dyn_cast<RetainValueInst *>()) {
951+
Callbacks.DeleteInst(RVI);
944952
return;
945953
}
946954

947-
SILInstruction *NewInst = Builder.emitDestroyAddrAndFold(Loc, Arg);
948-
Callbacks.CreatedNewInst(NewInst);
955+
Callbacks.CreatedNewInst(U.get<ReleaseValueInst *>());
949956
}
950957

951958
/// For each captured argument of PAI, decrement the ref count of the captured

test/SILOptimizer/sil_combine.sil

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ sil [global_init] @global_init_fun : $@convention(thin) () -> Builtin.RawPointer
3737

3838
sil @user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
3939

40+
sil @unknown : $@convention(thin) () -> ()
41+
4042
//////////////////////
4143
// Simple DCE Tests //
4244
//////////////////////
@@ -1755,7 +1757,6 @@ sil [readonly] @read_only_in : $@convention(thin) (@in SSS) -> ()
17551757
struct MyError : Error {
17561758
}
17571759

1758-
sil @unknown : $@convention(thin) () -> ()
17591760
sil [readonly] @readonly_throwing : $@convention(thin) (@owned ZZZ) -> (ZZZ, @error Error)
17601761

17611762
sil [readonly] @readonly_owned : $@convention(thin) (@owned B) -> @owned B
@@ -2266,36 +2267,6 @@ bb0(%0 : $Builtin.Int64, %1 : $Builtin.RawPointer, %2 : $Builtin.RawPointer):
22662267
return %13 : $()
22672268
}
22682269

2269-
// Make sure that we handle partial_apply captured arguments correctly.
2270-
sil @sil_combine_partial_apply_callee : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
2271-
2272-
// CHECK-LABEL: sil @sil_combine_partial_apply_caller : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> () {
2273-
// CHECK: bb0([[ARG1:%.*]] : $*Builtin.NativeObject, [[ARG2:%.*]] : $*Builtin.NativeObject, [[ARG3:%.*]] : $Builtin.NativeObject, [[ARG4:%.*]] : $Builtin.NativeObject, [[ARG5:%.*]] : $Builtin.NativeObject):
2274-
// CHECK-NEXT: strong_retain [[ARG3]]
2275-
// CHECK-NEXT: strong_retain [[ARG4]]
2276-
// CHECK-NEXT: strong_retain [[ARG5]]
2277-
// CHECK-NEXT: strong_release [[ARG3]]
2278-
// CHECK-NEXT: strong_release [[ARG4]]
2279-
// CHECK-NEXT: strong_release [[ARG5]]
2280-
// CHECK-NEXT: strong_release [[ARG4]]
2281-
// CHECK-NEXT: destroy_addr [[ARG1]]
2282-
sil @sil_combine_partial_apply_caller : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> () {
2283-
bb0(%0 : $*Builtin.NativeObject, %1 : $*Builtin.NativeObject, %2 : $Builtin.NativeObject, %3 : $Builtin.NativeObject, %4 : $Builtin.NativeObject):
2284-
%100 = function_ref @sil_combine_partial_apply_callee : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
2285-
%101 = alloc_stack $Builtin.NativeObject
2286-
copy_addr %0 to [initialization] %101 : $*Builtin.NativeObject
2287-
strong_retain %2 : $Builtin.NativeObject
2288-
strong_retain %3 : $Builtin.NativeObject
2289-
strong_retain %4 : $Builtin.NativeObject
2290-
%102 = partial_apply %100(%101, %1, %2, %3, %4) : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
2291-
strong_release %102 : $@callee_owned () -> ()
2292-
strong_release %3 : $Builtin.NativeObject
2293-
destroy_addr %0 : $*Builtin.NativeObject
2294-
dealloc_stack %101 : $*Builtin.NativeObject
2295-
%9999 = tuple()
2296-
return %9999 : $()
2297-
}
2298-
22992270
// CHECK-LABEL: sil @cmp_zext_peephole
23002271
// CHECK: bb0([[Arg1:%.*]] : $Builtin.Word, [[Arg2:%.*]] : $Builtin.Word):
23012272
// CHECK: [[ZA1:%.*]] = builtin "zextOrBitCast_Word_Int64"([[Arg1]] : $Builtin.Word) : $Builtin.Int64
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last -sil-combine-disable-alloc-stack-opts | %FileCheck %s
2+
3+
import Builtin
4+
5+
//////////////////
6+
// Declarations //
7+
//////////////////
8+
9+
sil @unknown : $@convention(thin) () -> ()
10+
11+
/////////////////////////////////
12+
// Tests for SILCombinerApply. //
13+
/////////////////////////////////
14+
15+
// Make sure that we handle partial_apply captured arguments correctly.
16+
//
17+
// We use custom types here to make it easier to pattern match with FileCheck.
18+
struct S1 { var x: Builtin.NativeObject }
19+
struct S2 { var x: Builtin.NativeObject }
20+
struct S3 { var x: Builtin.NativeObject }
21+
struct S4 { var x: Builtin.NativeObject }
22+
struct S5 { var x: Builtin.NativeObject }
23+
struct S6 { var x: Builtin.NativeObject }
24+
struct S7 { var x: Builtin.NativeObject }
25+
struct S8 { var x: Builtin.NativeObject }
26+
sil @sil_combine_partial_apply_callee : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
27+
28+
// *NOTE PLEASE READ*. If this test case looks funny to you, it is b/c partial
29+
// apply is funny. Specifically, even though a partial apply has the conventions
30+
// of the function on it, arguments to the partial apply (that will be passed
31+
// off to the function) must /always/ be passed in at +1. This is because the
32+
// partial apply is building up a boxed aggregate to send off to the closed over
33+
// function. Of course when you call the function, the proper conventions will
34+
// be used.
35+
//
36+
// CHECK-LABEL: sil @sil_combine_dead_partial_apply : $@convention(thin) (@in S2, @in S4, @inout S5, S6, @owned S7, @guaranteed S8) -> () {
37+
// CHECK: bb0([[IN_ARG:%.*]] : $*S2, [[INGUARANTEED_ARG:%.*]] : $*S4, [[INOUT_ARG:%.*]] : $*S5, [[UNOWNED_ARG:%.*]] : $S6, [[OWNED_ARG:%.*]] : $S7, [[GUARANTEED_ARG:%.*]] : $S8):
38+
//
39+
// CHECK: function_ref unknown
40+
// CHECK: [[UNKNOWN_FUNC:%.*]] = function_ref @unknown
41+
// CHECK-NEXT: [[IN_ADDRESS:%.*]] = alloc_stack $S1
42+
// CHECK-NEXT: [[INGUARANTEED_ADDRESS:%.*]] = alloc_stack $S3
43+
//
44+
// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
45+
//
46+
// Then make sure that the destroys are placed after the destroy_value of the
47+
// partial_apply (which is after this apply)...
48+
// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
49+
//
50+
// CHECK-NEXT: destroy_addr [[IN_ADDRESS]]
51+
// CHECK-NEXT: destroy_addr [[IN_ARG]]
52+
// CHECK-NEXT: destroy_addr [[INGUARANTEED_ADDRESS]]
53+
// CHECK-NEXT: destroy_addr [[INGUARANTEED_ARG]]
54+
// CHECK-NEXT: release_value [[UNOWNED_ARG]]
55+
// CHECK-NEXT: release_value [[OWNED_ARG]]
56+
// CHECK-NEXT: release_value [[GUARANTEED_ARG]]
57+
//
58+
// ... but before the function epilog.
59+
// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
60+
// CHECK-NEXT: dealloc_stack [[INGUARANTEED_ADDRESS]]
61+
// CHECK-NEXT: dealloc_stack [[IN_ADDRESS]]
62+
// CHECK-NEXT: tuple
63+
// CHECK-NEXT: return
64+
// CHECK-NEXT: } // end sil function 'sil_combine_dead_partial_apply'
65+
sil @sil_combine_dead_partial_apply : $@convention(thin) (@in S2, @in S4, @inout S5, S6, @owned S7, @guaranteed S8) -> () {
66+
bb0(%1 : $*S2, %2 : $*S4, %4 : $*S5, %5 : $S6, %6 : $S7, %7 : $S8):
67+
%8 = function_ref @unknown : $@convention(thin) () -> ()
68+
%9 = function_ref @sil_combine_partial_apply_callee : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
69+
70+
// This is for the @in alloc_stack case.
71+
%10 = alloc_stack $S1
72+
// This is for the @in_guaranteed alloc_stack case.
73+
%11 = alloc_stack $S3
74+
75+
// Marker of space in between the alloc_stack and the partial_apply
76+
apply %8() : $@convention(thin) () -> ()
77+
78+
// Now call the partial apply. We use the "unknown" function call after the
79+
// partial apply to ensure that we are truly placing releases at the partial
80+
// applies release rather than right afterwards.
81+
%102 = partial_apply %9(%10, %1, %11, %2, %4, %5, %6, %7) : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
82+
83+
// Marker of space in between partial_apply and the release of %102.
84+
apply %8() : $@convention(thin) () -> ()
85+
86+
strong_release %102 : $@callee_owned () -> ()
87+
88+
apply %8() : $@convention(thin) () -> ()
89+
90+
// Epilog.
91+
92+
// Cleanup the stack locations.
93+
dealloc_stack %11 : $*S3
94+
dealloc_stack %10 : $*S1
95+
96+
%9999 = tuple()
97+
return %9999 : $()
98+
}

0 commit comments

Comments
 (0)