Skip to content

Commit 53f424e

Browse files
eecksteineeckstein
committed
TempRValueOptimization: respect deinit-barriers when hoisting destroys
Fixes a mis-compile which causes a deinit to be called earlier than expected. rdar://162299994
1 parent 309b93c commit 53f424e

File tree

2 files changed

+205
-18
lines changed

2 files changed

+205
-18
lines changed

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

Lines changed: 92 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,21 @@ private func removeTempRValues(in function: Function, keepDebugInfo: Bool, _ con
6969
private func tryEliminate(copy: CopyLikeInstruction, keepDebugInfo: Bool, _ context: FunctionPassContext) {
7070

7171
guard let (allocStack, lastUseOfAllocStack) =
72-
getRemovableAllocStackDestination(of: copy, keepDebugInfo: keepDebugInfo, context) else {
72+
getRemovableAllocStackDestination(of: copy, keepDebugInfo: keepDebugInfo, context),
73+
let needHoistDestroys = needHoistDestroys(of: allocStack, after: lastUseOfAllocStack, copy: copy, context)
74+
else {
7375
return
7476
}
7577

7678
guard extendAccessScopes(beyond: lastUseOfAllocStack, copy: copy, context) else {
7779
return
7880
}
7981

80-
if copy.isTakeOfSource {
82+
if needHoistDestroys {
83+
// Hoist destroy_addrs after the last use, because between the last use and the original destroy
84+
// the source is modified.
8185
hoistDestroy(of: allocStack, after: lastUseOfAllocStack, context)
82-
} else {
86+
} else if !copy.isTakeOfSource {
8387
removeDestroys(of: allocStack, context)
8488
}
8589

@@ -140,21 +144,6 @@ private func getRemovableAllocStackDestination(
140144
return nil
141145
}
142146

143-
// We cannot insert the destroy of `copy.source` after `lastUseOfAllocStack` if `copy.source` is
144-
// re-initialized by exactly this instruction.
145-
// This is a corner case, but can happen if `lastUseOfAllocStack` is a `copy_addr`. Example:
146-
// ```
147-
// copy_addr [take] %src to [init] %allocStack // `copy`
148-
// copy_addr [take] %allocStack to [init] %src // `lastUseOfAllocStack`
149-
// ```
150-
if copy.isTakeOfSource,
151-
lastUseOfAllocStack != copy,
152-
!(lastUseOfAllocStack is DestroyAddrInst),
153-
lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis)
154-
{
155-
return nil
156-
}
157-
158147
// Bail if in non-OSSA the `allocStack` is destroyed in a non-obvious way, e.g. by
159148
// ```
160149
// %x = load %allocStack // looks like a load, but is a `load [take]`
@@ -171,6 +160,91 @@ private func getRemovableAllocStackDestination(
171160
return (allocStack, lastUseOfAllocStack)
172161
}
173162

163+
/// Returns true if the final `destroy_addr`s need to be hoisted to the last use of the `allocStack`.
164+
/// This is required if the copy-source is re-initialized inbetween, e.g.
165+
/// ```
166+
/// copy_addr %source, %allocStack
167+
/// ...
168+
/// last_use(%allocStack)
169+
/// ... <- the destroy_addr needs to be moved here
170+
/// store %newValue to [init] %source
171+
/// ...
172+
/// destroy_addr %allocStack
173+
/// ```
174+
/// Returns nil if hoisting is needed but not possible.
175+
private func needHoistDestroys(of allocStack: AllocStackInst,
176+
after lastUseOfAllocStack: Instruction,
177+
copy: CopyLikeInstruction,
178+
_ context: FunctionPassContext
179+
) -> Bool? {
180+
guard copy.isTakeOfSource else {
181+
return false
182+
}
183+
if lastUseOfAllocStack is DestroyAddrInst {
184+
assert(!copy.parentFunction.hasOwnership, "should not treat destroy_addr as uses in OSSA")
185+
return false
186+
}
187+
188+
// We cannot insert the destroy of `copy.source` after `lastUseOfAllocStack` if `copy.source` is
189+
// re-initialized by exactly this instruction.
190+
// This is a corner case, but can happen if `lastUseOfAllocStack` is a `copy_addr`. Example:
191+
// ```
192+
// copy_addr [take] %src to [init] %allocStack // `copy`
193+
// copy_addr [take] %allocStack to [init] %src // `lastUseOfAllocStack`
194+
// ```
195+
if lastUseOfAllocStack != copy, lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis) {
196+
return nil
197+
}
198+
199+
if mayWrite(to: copy.sourceAddress, after: lastUseOfAllocStack, beforeDestroysOf: allocStack, context) {
200+
if hasDestroyBarrier(after: lastUseOfAllocStack, beforeDestroysOf: allocStack, context) {
201+
return nil
202+
}
203+
return true
204+
}
205+
return false
206+
}
207+
208+
private func mayWrite(to source: Value,
209+
after lastUse: Instruction,
210+
beforeDestroysOf allocStack: AllocStackInst,
211+
_ context: FunctionPassContext
212+
) -> Bool {
213+
return visitInstructions(after: lastUse, beforeDestroysOf: allocStack, context) { inst in
214+
inst.mayWrite(toAddress: source, context.aliasAnalysis)
215+
}
216+
}
217+
218+
private func hasDestroyBarrier(after lastUse: Instruction,
219+
beforeDestroysOf allocStack: AllocStackInst,
220+
_ context: FunctionPassContext
221+
) -> Bool {
222+
return visitInstructions(after: lastUse, beforeDestroysOf: allocStack, context) { inst in
223+
inst.isBarrierForDestroy(of: allocStack.type, context)
224+
}
225+
}
226+
227+
private func visitInstructions(after lastUse: Instruction,
228+
beforeDestroysOf allocStack: AllocStackInst,
229+
_ context: FunctionPassContext,
230+
_ predicate: (Instruction) -> Bool
231+
) -> Bool {
232+
var worklist = InstructionWorklist(context)
233+
defer { worklist.deinitialize() }
234+
235+
for destroy in allocStack.uses.users(ofType: DestroyAddrInst.self) {
236+
worklist.pushPredecessors(of: destroy, ignoring: lastUse)
237+
}
238+
239+
while let inst = worklist.pop() {
240+
if predicate(inst) {
241+
return true
242+
}
243+
worklist.pushPredecessors(of: inst, ignoring: lastUse)
244+
}
245+
return false
246+
}
247+
174248
// We need to hoist the destroy of the stack location up to its last use because the source of the copy
175249
// could be re-initialized between the last use and the destroy.
176250
private func hoistDestroy(of allocStack: AllocStackInst, after lastUse: Instruction, _ context: FunctionPassContext) {

‎test/SILOptimizer/temp_rvalue_opt_ossa.sil‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ sil [ossa] @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guara
6666
sil [ossa] @inguaranteed_user_without_result_MOS : $@convention(thin) (@in_guaranteed MOS) -> ()
6767

6868
sil [ossa] @consuming_indirect_arg : $@convention(thin) (@in Klass) -> ()
69+
sil @deinit_barrier : $@convention(thin) () -> ()
6970

7071
sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
7172
bb0(%0 : $*Klass):
@@ -1677,3 +1678,115 @@ bb0(%0 : $*Klass):
16771678
return %9 : $()
16781679
}
16791680

1681+
// CHECK-LABEL: sil [ossa] @dont_hoist_destroy :
1682+
// CHECK-OPT-NOT: alloc_stack
1683+
// CHECK-OPT-NOT: copy_addr
1684+
// CHECK: bb2:
1685+
// CHECK-OPT-NEXT: destroy_addr %0
1686+
// CHECK-LABEL: } // end sil function 'dont_hoist_destroy'
1687+
sil [ossa] @dont_hoist_destroy : $@convention(thin) (@in Klass) -> () {
1688+
bb0(%0 : $*Klass):
1689+
%1 = alloc_stack [lexical] [var_decl] $Klass
1690+
copy_addr [take] %0 to [init] %1
1691+
br bb1
1692+
bb1:
1693+
%5 = function_ref @deinit_barrier : $@convention(thin) () -> ()
1694+
%6 = apply %5() : $@convention(thin) () -> ()
1695+
br bb2
1696+
bb2:
1697+
destroy_addr %1
1698+
dealloc_stack %1
1699+
%9 = tuple ()
1700+
return %9
1701+
}
1702+
1703+
// CHECK-LABEL: sil [ossa] @hoist_destroy :
1704+
// CHECK: bb0(%0 : $*Klass, %1 : @owned $Klass):
1705+
// CHECK-OPT-NEXT: destroy_addr %0
1706+
// CHECK-OPT-NOT: alloc_stack
1707+
// CHECK-OPT-NOT: copy_addr
1708+
// CHECK-OPT-NOT: destroy_addr
1709+
// CHECK-LABEL: } // end sil function 'hoist_destroy'
1710+
sil [ossa] @hoist_destroy : $@convention(thin) (@inout Klass, @owned Klass) -> () {
1711+
bb0(%0 : $*Klass, %1 : @owned $Klass):
1712+
%2 = alloc_stack [lexical] [var_decl] $Klass
1713+
copy_addr [take] %0 to [init] %2
1714+
br bb1
1715+
bb1:
1716+
store %1 to [init] %0
1717+
br bb2
1718+
bb2:
1719+
destroy_addr %2
1720+
dealloc_stack %2
1721+
%9 = tuple ()
1722+
return %9
1723+
}
1724+
1725+
// CHECK-LABEL: sil [ossa] @hoist_destroy2 :
1726+
// CHECK: bb0(%0 : $*Klass, %1 : @owned $Klass, %2 : $*Int, %3 : $Int):
1727+
// CHECK-OPT-NEXT: destroy_addr %0
1728+
// CHECK-OPT-NOT: alloc_stack
1729+
// CHECK-OPT-NOT: copy_addr
1730+
// CHECK-OPT-NOT: destroy_addr
1731+
// CHECK-LABEL: } // end sil function 'hoist_destroy2'
1732+
sil [ossa] @hoist_destroy2 : $@convention(thin) (@inout Klass, @owned Klass, @inout_aliasable Int, Int) -> () {
1733+
bb0(%0 : $*Klass, %1 : @owned $Klass, %2 : $*Int, %3 : $Int):
1734+
%4 = alloc_stack [lexical] [var_decl] $Klass
1735+
copy_addr [take] %0 to [init] %4
1736+
br bb1
1737+
bb1:
1738+
store %1 to [init] %0
1739+
store %3 to [trivial] %2
1740+
br bb2
1741+
bb2:
1742+
destroy_addr %4
1743+
dealloc_stack %4
1744+
%9 = tuple ()
1745+
return %9
1746+
}
1747+
1748+
// CHECK-LABEL: sil [ossa] @cant_hoist_destroy :
1749+
// CHECK: bb0(%0 : $*Klass, %1 : @owned $Klass):
1750+
// CHECK-NEXT: %2 = alloc_stack
1751+
// CHECK: bb2:
1752+
// CHECK-NEXT: destroy_addr %2
1753+
// CHECK-LABEL: } // end sil function 'cant_hoist_destroy'
1754+
sil [ossa] @cant_hoist_destroy : $@convention(thin) (@inout Klass, @owned Klass) -> () {
1755+
bb0(%0 : $*Klass, %1 : @owned $Klass):
1756+
%2 = alloc_stack [lexical] [var_decl] $Klass
1757+
copy_addr [take] %0 to [init] %2
1758+
br bb1
1759+
bb1:
1760+
store %1 to [init] %0
1761+
%5 = function_ref @deinit_barrier : $@convention(thin) () -> ()
1762+
%6 = apply %5() : $@convention(thin) () -> ()
1763+
br bb2
1764+
bb2:
1765+
destroy_addr %2
1766+
dealloc_stack %2
1767+
%9 = tuple ()
1768+
return %9
1769+
}
1770+
1771+
// CHECK-LABEL: sil [ossa] @cant_hoist_nc_destroy :
1772+
// CHECK: bb0(%0 : $*NCWithDeinit, %1 : @owned $NCWithDeinit, %2 : $*Int, %3 : $Int):
1773+
// CHECK-NEXT: %4 = alloc_stack
1774+
// CHECK: bb2:
1775+
// CHECK-NEXT: destroy_addr %4
1776+
// CHECK-LABEL: } // end sil function 'cant_hoist_nc_destroy'
1777+
sil [ossa] @cant_hoist_nc_destroy : $@convention(thin) (@inout NCWithDeinit, @owned NCWithDeinit, @inout_aliasable Int, Int) -> () {
1778+
bb0(%0 : $*NCWithDeinit, %1 : @owned $NCWithDeinit, %2 : $*Int, %3 : $Int):
1779+
%4 = alloc_stack [lexical] [var_decl] $NCWithDeinit
1780+
copy_addr [take] %0 to [init] %4
1781+
br bb1
1782+
bb1:
1783+
store %1 to [init] %0
1784+
store %3 to [trivial] %2
1785+
br bb2
1786+
bb2:
1787+
destroy_addr %4
1788+
dealloc_stack %4
1789+
%9 = tuple ()
1790+
return %9
1791+
}
1792+

0 commit comments

Comments
 (0)