Merge pull request #85027 from eeckstein/fix-temp-rvalue-elimination

TempRValueOptimization: respect deinit-barriers when hoisting destroys

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift

@@ -69,17 +69,21 @@ private func removeTempRValues(in function: Function, keepDebugInfo: Bool, _ con
 private func tryEliminate(copy: CopyLikeInstruction, keepDebugInfo: Bool, _ context: FunctionPassContext) {
 
  guard let (allocStack, lastUseOfAllocStack) =
- getRemovableAllocStackDestination(of: copy, keepDebugInfo: keepDebugInfo, context) else {
+ getRemovableAllocStackDestination(of: copy, keepDebugInfo: keepDebugInfo, context),
+ let needHoistDestroys = needHoistDestroys(of: allocStack, after: lastUseOfAllocStack, copy: copy, context)
+ else {
  return
  }
 
  guard extendAccessScopes(beyond: lastUseOfAllocStack, copy: copy, context) else {
  return
  }
 
- if copy.isTakeOfSource {
+ if needHoistDestroys {
+ // Hoist destroy_addrs after the last use, because between the last use and the original destroy
+ // the source is modified.
  hoistDestroy(of: allocStack, after: lastUseOfAllocStack, context)
- } else {
+ } else if !copy.isTakeOfSource {
  removeDestroys(of: allocStack, context)
  }
 
@@ -140,21 +144,6 @@ private func getRemovableAllocStackDestination(
  return nil
  }
 
- // We cannot insert the destroy of `copy.source` after `lastUseOfAllocStack` if `copy.source` is
- // re-initialized by exactly this instruction.
- // This is a corner case, but can happen if `lastUseOfAllocStack` is a `copy_addr`. Example:
- // ```
- // copy_addr [take] %src to [init] %allocStack // `copy`
- // copy_addr [take] %allocStack to [init] %src // `lastUseOfAllocStack`
- // ```
- if copy.isTakeOfSource,
- lastUseOfAllocStack != copy,
- !(lastUseOfAllocStack is DestroyAddrInst),
- lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis)
- {
- return nil
- }
-
  // Bail if in non-OSSA the `allocStack` is destroyed in a non-obvious way, e.g. by
  // ```
  // %x = load %allocStack // looks like a load, but is a `load [take]`
@@ -171,6 +160,91 @@ private func getRemovableAllocStackDestination(
  return (allocStack, lastUseOfAllocStack)
 }
 
+/// Returns true if the final `destroy_addr`s need to be hoisted to the last use of the `allocStack`.
+/// This is required if the copy-source is re-initialized inbetween, e.g.
+/// ```
+/// copy_addr %source, %allocStack
+/// ...
+/// last_use(%allocStack)
+/// ... <- the destroy_addr needs to be moved here
+/// store %newValue to [init] %source
+/// ...
+/// destroy_addr %allocStack
+/// ```
+/// Returns nil if hoisting is needed but not possible.
+private func needHoistDestroys(of allocStack: AllocStackInst,
+ after lastUseOfAllocStack: Instruction,
+ copy: CopyLikeInstruction,
+ _ context: FunctionPassContext
+) -> Bool? {
+ guard copy.isTakeOfSource else {
+ return false
+ }
+ if lastUseOfAllocStack is DestroyAddrInst {
+ assert(!copy.parentFunction.hasOwnership, "should not treat destroy_addr as uses in OSSA")
+ return false
+ }
+
+ // We cannot insert the destroy of `copy.source` after `lastUseOfAllocStack` if `copy.source` is
+ // re-initialized by exactly this instruction.
+ // This is a corner case, but can happen if `lastUseOfAllocStack` is a `copy_addr`. Example:
+ // ```
+ // copy_addr [take] %src to [init] %allocStack // `copy`
+ // copy_addr [take] %allocStack to [init] %src // `lastUseOfAllocStack`
+ // ```
+ if lastUseOfAllocStack != copy, lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis) {
+ return nil
+ }
+
+ if mayWrite(to: copy.sourceAddress, after: lastUseOfAllocStack, beforeDestroysOf: allocStack, context) {
+ if hasDestroyBarrier(after: lastUseOfAllocStack, beforeDestroysOf: allocStack, context) {
+ return nil
+ }
+ return true
+ }
+ return false
+}
+
+private func mayWrite(to source: Value,
+ after lastUse: Instruction,
+ beforeDestroysOf allocStack: AllocStackInst,
+ _ context: FunctionPassContext
+) -> Bool {
+ return visitInstructions(after: lastUse, beforeDestroysOf: allocStack, context) { inst in
+ inst.mayWrite(toAddress: source, context.aliasAnalysis)
+ }
+}
+
+private func hasDestroyBarrier(after lastUse: Instruction,
+ beforeDestroysOf allocStack: AllocStackInst,
+ _ context: FunctionPassContext
+) -> Bool {
+ return visitInstructions(after: lastUse, beforeDestroysOf: allocStack, context) { inst in
+ inst.isBarrierForDestroy(of: allocStack.type, context)
+ }
+}
+
+private func visitInstructions(after lastUse: Instruction,
+ beforeDestroysOf allocStack: AllocStackInst,
+ _ context: FunctionPassContext,
+ _ predicate: (Instruction) -> Bool
+) -> Bool {
+ var worklist = InstructionWorklist(context)
+ defer { worklist.deinitialize() }
+
+ for destroy in allocStack.uses.users(ofType: DestroyAddrInst.self) {
+ worklist.pushPredecessors(of: destroy, ignoring: lastUse)
+ }
+
+ while let inst = worklist.pop() {
+ if predicate(inst) {
+ return true
+ }
+ worklist.pushPredecessors(of: inst, ignoring: lastUse)
+ }
+ return false
+}
+
 // We need to hoist the destroy of the stack location up to its last use because the source of the copy
 // could be re-initialized between the last use and the destroy.
 private func hoistDestroy(of allocStack: AllocStackInst, after lastUse: Instruction, _ context: FunctionPassContext) {

SwiftCompilerSources/Sources/Optimizer/Utilities/FunctionTest.swift

@@ -51,7 +51,8 @@ public func registerOptimizerTests() {
  localVariableReachableUsesTest,
  localVariableReachingAssignmentsTest,
  rangeOverlapsPathTest,
- variableIntroducerTest
+ variableIntroducerTest,
+ destroyBarrierTest
  )
 
  // Finally register the thunk they all call through.

SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift

@@ -546,6 +546,54 @@ extension Instruction {
  // Eventually we want to replace the SILCombine implementation with this one.
  return nil
  }
+
+ /// Returns true if a destroy of `type` must not be moved across this instruction.
+ func isBarrierForDestroy(of type: Type, _ context: some Context) -> Bool {
+ let instEffects = memoryEffects
+ if instEffects == .noEffects || type.isTrivial(in: parentFunction) {
+ return false
+ }
+ guard type.isMoveOnly else {
+ // Non-trivial copyable types only have to consider deinit-barriers for classes.
+ return isDeinitBarrier(context.calleeAnalysis)
+ }
+
+ // Check side-effects of non-copyable deinits.
+
+ guard let nominal = type.nominal else {
+ return true
+ }
+ if nominal.valueTypeDestructor != nil {
+ guard let deinitFunc = context.lookupDeinit(ofNominal: nominal) else {
+ return true
+ }
+ let destructorEffects = deinitFunc.getSideEffects().memory
+ if instEffects.write || destructorEffects.write {
+ return true
+ }
+ }
+
+ switch nominal {
+ case is StructDecl:
+ guard let fields = type.getNominalFields(in: parentFunction) else {
+ return true
+ }
+ return fields.contains { isBarrierForDestroy(of: $0, context) }
+ case is EnumDecl:
+ guard let enumCases = type.getEnumCases(in: parentFunction) else {
+ return true
+ }
+ return enumCases.contains {
+ if let payload = $0.payload {
+ return isBarrierForDestroy(of: payload, context)
+ }
+ return false
+ }
+ default:
+ return true
+ }
+ }
+
 }
 
 // Match the pattern:
@@ -1133,3 +1181,14 @@ func cloneAndSpecializeFunction(from originalFunction: Function,
  defer { cloner.deinitialize() }
  cloner.cloneFunctionBody()
 }
+
+let destroyBarrierTest = FunctionTest("destroy_barrier") { function, arguments, context in
+ let type = arguments.takeValue().type
+ for inst in function.instructions {
+ if inst.isBarrierForDestroy(of: type, context) {
+ print("barrier: \(inst)")
+ } else {
+ print("transparent: \(inst)")
+ }
+ }
+}