Skip to content

Commit b6b4a7b

Browse files
meg-guptameg-gupta
authored
Merge pull request swiftlang#79841 from meg-gupta/fixsemanticarc
Fix two optimizer issues
2 parents b038376 + cd684a3 commit b6b4a7b

File tree

4 files changed

+175
-9
lines changed

4 files changed

+175
-9
lines changed

‎SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DestroyHoisting.swift

+9-3
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,13 @@ private func optimize(value: Value, _ context: FunctionPassContext) {
8888
return
8989
}
9090

91-
var hoistableDestroys = selectHoistableDestroys(of: value, context)
91+
var (foundDestroys, hoistableDestroys) = selectHoistableDestroys(of: value, context)
9292
defer { hoistableDestroys.deinitialize() }
9393

94+
guard foundDestroys else {
95+
return
96+
}
97+
9498
guard var minimalLiverange = InstructionRange(withLiverangeOf: value, ignoring: hoistableDestroys, context) else {
9599
return
96100
}
@@ -99,12 +103,13 @@ private func optimize(value: Value, _ context: FunctionPassContext) {
99103
hoistDestroys(of: value, toEndOf: minimalLiverange, restrictingTo: &hoistableDestroys, context)
100104
}
101105

102-
private func selectHoistableDestroys(of value: Value, _ context: FunctionPassContext) -> InstructionSet {
106+
private func selectHoistableDestroys(of value: Value, _ context: FunctionPassContext) -> (Bool, InstructionSet) {
103107
// Also includes liveranges of copied values and values stored to memory.
104108
var forwardExtendedLiverange = InstructionRange(withForwardExtendedLiverangeOf: value, context)
105109
defer { forwardExtendedLiverange.deinitialize() }
106110

107111
let deadEndBlocks = context.deadEndBlocks
112+
var foundDestroys = false
108113
var hoistableDestroys = InstructionSet(context)
109114

110115
for use in value.uses {
@@ -114,10 +119,11 @@ private func selectHoistableDestroys(of value: Value, _ context: FunctionPassCon
114119
// TODO: once we have complete OSSA lifetimes we don't need to handle dead-end blocks.
115120
!deadEndBlocks.isDeadEnd(destroy.parentBlock)
116121
{
122+
foundDestroys = true
117123
hoistableDestroys.insert(destroy)
118124
}
119125
}
120-
return hoistableDestroys
126+
return (foundDestroys, hoistableDestroys)
121127
}
122128

123129
private func hoistDestroys(of value: Value,

‎lib/SILOptimizer/SemanticARC/CopyValueOpts.cpp

+5-6
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,11 @@ static bool isUseBetweenInstAndBlockEnd(
388388
static bool tryJoinIfDestroyConsumingUseInSameBlock(
389389
SemanticARCOptVisitor &ctx, CopyValueInst *cvi, DestroyValueInst *dvi,
390390
SILValue operand, Operand *singleCVIConsumingUse) {
391+
// Pointer escapes propagate values ways that may not be discoverable.
392+
// If \p cvi or \p operand has escaped, then do not optimize.
393+
if (findPointerEscape(cvi) || findPointerEscape(operand)) {
394+
return false;
395+
}
391396
// First see if our destroy_value is in between singleCVIConsumingUse and the
392397
// end of block. If this is not true, then we know the destroy_value must be
393398
// /before/ our singleCVIConsumingUse meaning that by joining the lifetimes,
@@ -517,12 +522,6 @@ static bool tryJoinIfDestroyConsumingUseInSameBlock(
517522
}))
518523
return false;
519524
}
520-
// Check whether the uses considered immediately above are all effectively
521-
// instantaneous uses. Pointer escapes propagate values ways that may not be
522-
// discoverable.
523-
if (findPointerEscape(operand)) {
524-
return false;
525-
}
526525

527526
// Ok, we now know that we can eliminate this value.
528527
LLVM_DEBUG(llvm::dbgs()

‎test/SILOptimizer/definite-init-convert-to-escape.swift

+2
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,12 @@ public func returnOptionalEscape() -> (() ->())?
9292
// NOPEEPHOLE: switch_enum [[V1]] : $Optional<{{.*}}>, case #Optional.some!enumelt: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]]
9393
//
9494
// NOPEEPHOLE: [[SOME_BB]]([[V2:%.*]]: $@callee_guaranteed () -> ()):
95+
// NOPEEPHOLE-NEXT: strong_retain [[V2]]
9596
// NOPEEPHOLE-NEXT: [[CVT:%.*]] = convert_escape_to_noescape [[V2]]
9697
// NOPEEPHOLE-NEXT: [[SOME:%.*]] = enum $Optional<{{.*}}>, #Optional.some!enumelt, [[V2]]
9798
// NOPEEPHOLE-NEXT: [[MDI:%.*]] = mark_dependence [[CVT]] : $@noescape @callee_guaranteed () -> () on [[SOME]] : $Optional<@callee_guaranteed () -> ()>
9899
// NOPEEPHOLE-NEXT: [[NOESCAPE_SOME:%.*]] = enum $Optional<{{.*}}>, #Optional.some!enumelt, [[MDI]]
100+
// NOPEEPHOLE-NEXT: strong_release [[V2]]
99101
// NOPEEPHOLE-NEXT: br bb2([[NOESCAPE_SOME]] : $Optional<{{.*}}>, [[SOME]] : $Optional<{{.*}}>, [[SOME]] : $Optional<{{.*}}>)
100102
//
101103
// NOPEEPHOLE: bb2([[NOESCAPE_SOME:%.*]] : $Optional<{{.*}}>, [[SOME1:%.*]] : $Optional<{{.*}}>, [[SOME:%.*]] : $Optional<{{.*}}>):

‎test/SILOptimizer/rdar146142041.sil

+159
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
// RUN: %target-sil-opt -enable-sil-verify-all %s -semantic-arc-opts -destroy-hoisting | %FileCheck %s
2+
3+
import Swift
4+
5+
class Klass {
6+
init()
7+
}
8+
9+
struct NonTrivial {
10+
@_hasStorage let k: Klass { get }
11+
init(k: Klass)
12+
}
13+
14+
sil @foo : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
15+
16+
// test1 is the reduction from rdar://146142041. test2 and test3 are mutations of test1 and may not be created from swift code in practice
17+
// CHECK-LABEL: sil [ossa] @test1 :
18+
// CHECK: [[PA:%.*]] = partial_apply
19+
// CHECK: [[COPY:%.*]] = copy_value [[PA]]
20+
// CHECK-LABEL: } // end sil function 'test1'
21+
sil [ossa] @test1 : $@convention(thin) () -> () {
22+
bb0:
23+
cond_br undef, bb2, bb1
24+
25+
bb1:
26+
%1 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.none!enumelt
27+
br bb7(%1)
28+
29+
bb2:
30+
br bb3
31+
32+
bb3:
33+
%4 = alloc_box ${ var Optional<NonTrivial> }
34+
%5 = begin_borrow [lexical] %4
35+
%6 = project_box %5, 0
36+
inject_enum_addr %6, #Optional.none!enumelt
37+
cond_br undef, bb4, bb5
38+
39+
bb4:
40+
unreachable
41+
42+
bb5:
43+
br bb6
44+
45+
bb6:
46+
%11 = function_ref @foo : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
47+
%12 = partial_apply [callee_guaranteed] %11(%6) : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
48+
%13 = copy_value %12
49+
%14 = convert_escape_to_noescape %13 to $@noescape @callee_guaranteed () -> ()
50+
%15 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, %13
51+
end_borrow %5
52+
destroy_value %12
53+
destroy_value %4
54+
destroy_value %14
55+
br bb7(%15)
56+
57+
bb7(%21 : @owned $Optional<@callee_guaranteed () -> ()>):
58+
destroy_value %21
59+
br bb8
60+
61+
bb8:
62+
%24 = tuple ()
63+
return %24
64+
}
65+
66+
// CHECK-LABEL: sil [ossa] @test2 :
67+
// CHECK: [[PA:%.*]] = partial_apply
68+
// CHECK: [[COPY:%.*]] = copy_value [[PA]]
69+
// CHECK-LABEL: } // end sil function 'test2'
70+
sil [ossa] @test2 : $@convention(thin) () -> () {
71+
bb0:
72+
cond_br undef, bb2, bb1
73+
74+
bb1:
75+
%1 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.none!enumelt
76+
br bb7(%1)
77+
78+
bb2:
79+
br bb3
80+
81+
bb3:
82+
%4 = alloc_box ${ var Optional<NonTrivial> }
83+
%5 = begin_borrow [lexical] %4
84+
%6 = project_box %5, 0
85+
inject_enum_addr %6, #Optional.none!enumelt
86+
cond_br undef, bb4, bb5
87+
88+
bb4:
89+
unreachable
90+
91+
bb5:
92+
br bb6
93+
94+
bb6:
95+
%11 = function_ref @foo : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
96+
%12 = partial_apply [callee_guaranteed] %11(%6) : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
97+
%13 = copy_value %12
98+
destroy_value %12
99+
%14 = convert_escape_to_noescape %13 to $@noescape @callee_guaranteed () -> ()
100+
%15 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, %13
101+
end_borrow %5
102+
destroy_value %4
103+
destroy_value %14
104+
br bb7(%15)
105+
106+
bb7(%21 : @owned $Optional<@callee_guaranteed () -> ()>):
107+
destroy_value %21
108+
br bb8
109+
110+
bb8:
111+
%24 = tuple ()
112+
return %24
113+
}
114+
115+
// Ensure no ownership verification error
116+
sil [ossa] @test3 : $@convention(thin) () -> () {
117+
bb0:
118+
cond_br undef, bb2, bb1
119+
120+
bb1:
121+
%1 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.none!enumelt
122+
br bb7(%1)
123+
124+
bb2:
125+
br bb3
126+
127+
bb3:
128+
%4 = alloc_box ${ var Optional<NonTrivial> }
129+
%5 = begin_borrow [lexical] %4
130+
%6 = project_box %5, 0
131+
inject_enum_addr %6, #Optional.none!enumelt
132+
cond_br undef, bb4, bb5
133+
134+
bb4:
135+
unreachable
136+
137+
bb5:
138+
br bb6
139+
140+
bb6:
141+
%11 = function_ref @foo : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
142+
%12 = partial_apply [callee_guaranteed] %11(%6) : $@convention(thin) (@inout_aliasable Optional<NonTrivial>) -> ()
143+
%13 = copy_value %12
144+
%14 = convert_escape_to_noescape %13 to $@noescape @callee_guaranteed () -> ()
145+
%15 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, %13
146+
end_borrow %5
147+
destroy_value %4
148+
destroy_value %12
149+
destroy_value %14
150+
br bb7(%15)
151+
152+
bb7(%21 : @owned $Optional<@callee_guaranteed () -> ()>):
153+
destroy_value %21
154+
br bb8
155+
156+
bb8:
157+
%24 = tuple ()
158+
return %24
159+
}

0 commit comments

Comments
 (0)