Merge pull request #81469 from eeckstein/temp-rvalue-elimination

TempRValueElimination: handle `store` instructions to the temporary stack location

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift

@@ -38,18 +38,51 @@ let tempRValueElimination = FunctionPass(name: "temp-rvalue-elimination") {
   (function: Function, context: FunctionPassContext) in
 
   for inst in function.instructions {
-    if let copyAddr = inst as? CopyAddrInst {
-      tryEliminate(copy: copyAddr, context)
+    switch inst {
+    case let copy as CopyAddrInst:
+      if copy.source == copy.destination {
+        // Remove identity copies which may have been created by an earlier iteration, where another `copy_addr`
+        // copied the `alloc_stack` back to the source location.
+        context.erase(instruction: copy)
+      } else {
+        tryEliminate(copy: copy, context)
+      }
+    case let store as StoreInst:
+      // Also handle `load`-`store` pairs which are basically the same thing as a `copy_addr`.
+      if let load = store.source as? LoadInst, load.uses.isSingleUse, load.parentBlock == store.parentBlock {
+        tryEliminate(copy: store, context)
+      }
+    default:
+      break
     }
   }
 }
 
-private func tryEliminate(copy: CopyAddrInst, _ context: FunctionPassContext) {
-  // Remove identity copies which may have been created by an earlier iteration, where another `copy_addr`
-  // copied the `alloc_stack` back to the source location.
-  if copy.source == copy.destination {
-    context.erase(instruction: copy)
-  }
+private protocol CopyLikeInstruction: Instruction {
+  var sourceAddress: Value { get }
+  var destinationAddress: Value { get }
+  var isTakeOfSource: Bool { get }
+  var isInitializationOfDestination: Bool { get }
+  var loadingInstruction: Instruction { get }
+}
+
+extension CopyAddrInst: CopyLikeInstruction {
+  var sourceAddress: Value { source }
+  var destinationAddress: Value { destination }
+  var loadingInstruction: Instruction { self }
+}
+
+// A `store` which has a `load` as source operand. This is basically the same as a `copy_addr`.
+extension StoreInst: CopyLikeInstruction {
+  var sourceAddress: Value { load.address }
+  var destinationAddress: Value { destination }
+  var isTakeOfSource: Bool { load.loadOwnership == .take }
+  var isInitializationOfDestination: Bool { storeOwnership != .assign }
+  var loadingInstruction: Instruction { load }
+  private var load: LoadInst { source as! LoadInst }
+}
+
+private func tryEliminate(copy: CopyLikeInstruction, _ context: FunctionPassContext) {
 
   guard let (allocStack, lastUseOfAllocStack) = getRemovableAllocStackDestination(of: copy, context) else {
     return
@@ -61,7 +94,7 @@ private func tryEliminate(copy: CopyAddrInst, _ context: FunctionPassContext) {
 
   if needToInsertDestroy(copy: copy, lastUseOfAllocStack: lastUseOfAllocStack) {
     Builder.insert(after: lastUseOfAllocStack, context) { builder in
-      builder.createDestroyAddr(address: copy.source)
+      builder.createDestroyAddr(address: copy.sourceAddress)
     }
   }
 
@@ -76,25 +109,25 @@ private func tryEliminate(copy: CopyAddrInst, _ context: FunctionPassContext) {
         assert(cai == lastUseOfAllocStack && cai.source == allocStack)
         cai.set(isTakeOfSource: false, context)
       }
-      use.set(to: copy.source, context)
+      use.set(to: copy.sourceAddress, context)
     case let load as LoadInst:
       if load.loadOwnership == .take, !copy.isTakeOfSource {
         // If the original copy is not taking its source, a `load` from the `allocStack` must
         // not take its source either.
         assert(load == lastUseOfAllocStack)
         load.set(ownership: .copy, context)
       }
-      use.set(to: copy.source, context);
+      use.set(to: copy.sourceAddress, context);
 
     default:
       // Note that no operations that may be handled by this default clause can destroy the `allocStack`.
       // This includes operations that load the value from memory and release it or cast the address
       // before destroying it.
-      use.set(to: copy.source, context);
+      use.set(to: copy.sourceAddress, context);
     }
   }
   context.erase(instruction: allocStack)
-  context.erase(instruction: copy)
+  context.erase(instructionIncludingAllUsers: copy.loadingInstruction)
 }
 
 /// Checks if the `copy` is copying into an `alloc_stack` which is removable:
@@ -105,17 +138,17 @@ private func tryEliminate(copy: CopyAddrInst, _ context: FunctionPassContext) {
 ///   %lastUseOfAllocStack = load %allocStack
 /// ```
 private func getRemovableAllocStackDestination(
-  of copy: CopyAddrInst, _ context: FunctionPassContext
+  of copy: CopyLikeInstruction, _ context: FunctionPassContext
 ) -> (allocStack: AllocStackInst, lastUseOfAllocStack: Instruction)? {
   guard copy.isInitializationOfDestination,
-        let allocStack = copy.destination as? AllocStackInst
+        let allocStack = copy.destinationAddress as? AllocStackInst
   else {
     return nil
   }
 
   // If the `allocStack` is lexical we can eliminate it if the source of the copy is lexical and
   // it is live for longer than the `allocStack`.
-  if allocStack.isLexical && !copy.source.accessBase.storageIsLexical {
+  if allocStack.isLexical && !copy.sourceAddress.accessBase.storageIsLexical {
     return nil
   }
 
@@ -149,7 +182,7 @@ private func getRemovableAllocStackDestination(
   if copy.isTakeOfSource,
      lastUseOfAllocStack != copy,
      !(lastUseOfAllocStack is DestroyAddrInst),
-     lastUseOfAllocStack.mayWrite(toAddress: copy.source, context.aliasAnalysis)
+     lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis)
   {
     return nil
   }
@@ -168,7 +201,7 @@ private func getRemovableAllocStackDestination(
 
 /// We need to insert a final destroy if the original `copy` is taking the source but the
 /// `lastUseOfAllocStack` is not taking the `alloc_stack`.
-private func needToInsertDestroy(copy: CopyAddrInst, lastUseOfAllocStack: Instruction) -> Bool {
+private func needToInsertDestroy(copy: CopyLikeInstruction, lastUseOfAllocStack: Instruction) -> Bool {
   if !copy.isTakeOfSource {
     return false
   }
@@ -177,13 +210,13 @@ private func needToInsertDestroy(copy: CopyAddrInst, lastUseOfAllocStack: Instru
     return true
   case let cai as CopyAddrInst:
     if cai.isTakeOfSource {
-      assert(cai.source == copy.destination, "copy_addr must be not take a projected address")
+      assert(cai.source == copy.destinationAddress, "copy_addr must be not take a projected address")
       return false
     }
     return true
   case let li as LoadInst:
     if li.loadOwnership == .take {
-      assert(li.address == copy.destination, "load must be not take a projected address")
+      assert(li.address == copy.destinationAddress, "load must be not take a projected address")
       return false
     }
     return true
@@ -247,7 +280,9 @@ private extension AllocStackInst {
 /// ```
 /// We must not replace %temp with %a after the `end_access`. Instead we try to move the `end_access`
 /// after the last use.
-private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyAddrInst, _ context: FunctionPassContext) -> Bool {
+private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyLikeInstruction,
+                                _ context: FunctionPassContext) -> Bool
+{
   var endAccessToMove: EndAccessInst? = nil
 
   for inst in InstructionList(first: copy.next) {
@@ -257,7 +292,7 @@ private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyAddrInst,
       if endAccessToMove != nil {
         return false
       }
-      if context.aliasAnalysis.mayAlias(copy.source, endAccess.beginAccess.address),
+      if context.aliasAnalysis.mayAlias(copy.sourceAddress, endAccess.beginAccess.address),
          // There cannot be any aliasing modifying accesses within the liverange of the `alloc_stack`,
          // because we would have cought this in `getLastUseWhileSourceIsNotModified`.
          // But there are cases where `aliasAnalysis.mayAlias` is less precise than `Instruction.mayWrite`.
@@ -301,7 +336,7 @@ private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyAddrInst,
 ///
 /// Unfortunately, we cannot simply use the destroy points as the lifetime end, because they can be in a
 /// different basic block. Instead we look for the last non-destroy, non-dealloc use.
-private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
+private func getLastUseWhileSourceIsNotModified(of copy: CopyLikeInstruction,
                                                 uses: InstructionSetWithCount,
                                                 _ context: FunctionPassContext) -> Instruction?
 {
@@ -313,7 +348,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
 
   // We already checked that the useful lifetime of the `alloc_stack` ends in the same block as the `copy`.
   // Therefore we can limit our search to the instructions of this block.
-  for inst in InstructionList(first: copy.next) {
+  for inst in InstructionList(first: copy.loadingInstruction.next) {
     if uses.contains(inst) {
       numUsesFound += 1
     }
@@ -327,7 +362,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
       // could occur _before_ the read of the `alloc_stack`.
       switch inst {
       case is FullApplySite, is YieldInst:
-        if inst.mayWrite(toAddress: copy.source, aliasAnalysis) {
+        if inst.mayWrite(toAddress: copy.sourceAddress, aliasAnalysis) {
           return nil
         }
         return inst
@@ -336,7 +371,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
       }
     }
 
-    if inst.mayWrite(toAddress: copy.source, aliasAnalysis) {
+    if inst.mayWrite(toAddress: copy.sourceAddress, aliasAnalysis) {
       return nil
     }
   }
@@ -348,9 +383,9 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
 /// Collects all uses of the `alloc_stack`.
 private struct UseCollector : AddressDefUseWalker {
   private(set) var uses: InstructionSetWithCount
-  private let copy: CopyAddrInst
+  private let copy: CopyLikeInstruction
 
-  init(copy: CopyAddrInst, _ context: FunctionPassContext) {
+  init(copy: CopyLikeInstruction, _ context: FunctionPassContext) {
     self.uses = InstructionSetWithCount(context)
     self.copy = copy
   }
@@ -467,7 +502,7 @@ private struct UseCollector : AddressDefUseWalker {
       if load.loadOwnership == .take,
           // Only accept `load [take]` if it takes the whole `alloc_stack`. A `load [take]` from
           // a projection would destroy only a part of the `alloc_stack` and we don't handle this.
-          load.address != copy.destination
+          load.address != copy.destinationAddress
       {
         return .abortWalk
       }
@@ -494,7 +529,7 @@ private struct UseCollector : AddressDefUseWalker {
         return .abortWalk
       }
       // As with `load [take]`, only accept `copy_addr [take]` if it takes the whole `alloc_stack`.
-      if copyFromStack.isTakeOfSource && copyFromStack.source != copy.destination {
+      if copyFromStack.isTakeOfSource && copyFromStack.source != copy.destinationAddress {
         return .abortWalk
       }
       uses.insert(copyFromStack)

SwiftCompilerSources/Sources/Optimizer/PassManager/Context.swift

@@ -140,7 +140,10 @@ extension MutatingContext {
     return _bridged.createBlockAfter(block.bridged).block
   }
 
-  func erase(instruction: Instruction) {
+  /// Removes and deletes `instruction`.
+  /// If `salvageDebugInfo` is true, compensating `debug_value` instructions are inserted for certain
+  /// kind of instructions.
+  func erase(instruction: Instruction, salvageDebugInfo: Bool = true) {
     if !instruction.isInStaticInitializer {
       verifyIsTransforming(function: instruction.parentFunction)
     }
@@ -152,7 +155,7 @@ extension MutatingContext {
     }
     notifyInstructionsChanged()
 
-    _bridged.eraseInstruction(instruction.bridged)
+    _bridged.eraseInstruction(instruction.bridged, salvageDebugInfo)
   }
 
   func erase(instructionIncludingAllUsers inst: Instruction) {
@@ -164,7 +167,9 @@ extension MutatingContext {
         erase(instructionIncludingAllUsers: use.instruction)
       }
     }
-    erase(instruction: inst)
+    // We rely that after deleting the instruction its operands have no users.
+    // Therefore `salvageDebugInfo` must be turned off because we cannot insert debug_value instructions.
+    erase(instruction: inst, salvageDebugInfo: false)
   }
 
   func erase<S: Sequence>(instructions: S) where S.Element: Instruction {

include/swift/SIL/SILInstructionWorklist.h

@@ -302,9 +302,12 @@ class SILInstructionWorklist : SILInstructionWorklistBase {
   // method to delete the given instruction.
   void eraseInstFromFunction(SILInstruction &instruction,
                              SILBasicBlock::iterator &iterator,
-                             bool addOperandsToWorklist = true) {
-    // Try to salvage debug info first.
-    swift::salvageDebugInfo(&instruction);
+                             bool addOperandsToWorklist = true,
+                             bool salvageDebugInfo = true) {
+    if (salvageDebugInfo) {
+      // Try to salvage debug info first.
+      swift::salvageDebugInfo(&instruction);
+    }
     // Then delete old debug users.
     for (auto result : instruction.getResults()) {
       while (!result->use_empty()) {
@@ -323,9 +326,11 @@ class SILInstructionWorklist : SILInstructionWorklistBase {
   }
 
   void eraseInstFromFunction(SILInstruction &instruction,
-                             bool addOperandsToWorklist = true) {
+                             bool addOperandsToWorklist = true,
+                             bool salvageDebugInfo = true) {
     SILBasicBlock::iterator nullIter;
-    return eraseInstFromFunction(instruction, nullIter, addOperandsToWorklist);
+    return eraseInstFromFunction(instruction, nullIter, addOperandsToWorklist,
+                                 salvageDebugInfo);
   }
 
   void eraseSingleInstFromFunction(SILInstruction &instruction,

include/swift/SILOptimizer/OptimizerBridging.h

@@ -234,7 +234,7 @@ struct BridgedPassContext {
   SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedBasicBlock splitBlockAfter(BridgedInstruction bridgedInst) const;
   SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedBasicBlock createBlockAfter(BridgedBasicBlock bridgedBlock) const;
   SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedBasicBlock appendBlock(BridgedFunction bridgedFunction) const;
-  BRIDGED_INLINE void eraseInstruction(BridgedInstruction inst) const;
+  BRIDGED_INLINE void eraseInstruction(BridgedInstruction inst, bool salvageDebugInfo) const;
   BRIDGED_INLINE void eraseBlock(BridgedBasicBlock block) const;
   static BRIDGED_INLINE void moveInstructionBefore(BridgedInstruction inst, BridgedInstruction beforeInst);
   bool tryOptimizeApplyOfPartialApply(BridgedInstruction closure) const;

include/swift/SILOptimizer/OptimizerBridgingImpl.h

@@ -27,6 +27,7 @@
 #include "swift/SILOptimizer/OptimizerBridging.h"
 #include "swift/SILOptimizer/PassManager/PassManager.h"
 #include "swift/SILOptimizer/Utils/InstOptUtils.h"
+#include "swift/SILOptimizer/Utils/DebugOptUtils.h"
 
 SWIFT_BEGIN_NULLABILITY_ANNOTATIONS
 
@@ -253,8 +254,8 @@ BridgedBasicBlock BridgedPassContext::appendBlock(BridgedFunction bridgedFunctio
   return {bridgedFunction.getFunction()->createBasicBlock()};
 }
 
-void BridgedPassContext::eraseInstruction(BridgedInstruction inst) const {
-  invocation->eraseInstruction(inst.unbridged());
+void BridgedPassContext::eraseInstruction(BridgedInstruction inst, bool salvageDebugInfo) const {
+  invocation->eraseInstruction(inst.unbridged(), salvageDebugInfo);
 }
 
 void BridgedPassContext::eraseBlock(BridgedBasicBlock block) const {

include/swift/SILOptimizer/PassManager/PassManager.h

@@ -138,7 +138,7 @@ class SwiftPassInvocation {
   void freeOperandSet(OperandSet *set);
 
   /// The top-level API to erase an instruction, called from the Swift pass.
-  void eraseInstruction(SILInstruction *inst);
+  void eraseInstruction(SILInstruction *inst, bool salvageDebugInfo);
 
   /// Called by the pass when changes are made to the SIL.
   void notifyChanges(SILAnalysis::InvalidationKind invalidationKind);

lib/SILOptimizer/SILCombiner/SILCombine.cpp

@@ -694,11 +694,13 @@ SILTransform *swift::createSILCombine() {
 //                          SwiftFunctionPassContext
 //===----------------------------------------------------------------------===//
 
-void SwiftPassInvocation::eraseInstruction(SILInstruction *inst) {
+void SwiftPassInvocation::eraseInstruction(SILInstruction *inst, bool salvageDebugInfo) {
   if (silCombiner) {
-    silCombiner->eraseInstFromFunction(*inst);
+    silCombiner->eraseInstFromFunction(*inst, /*addOperandsToWorklist=*/ true, salvageDebugInfo);
   } else {
-    swift::salvageDebugInfo(inst);
+    if (salvageDebugInfo) {
+      swift::salvageDebugInfo(inst);
+    }
     if (inst->isStaticInitializerInst()) {
       inst->getParent()->erase(inst, *getPassManager()->getModule());
     } else {

lib/SILOptimizer/SILCombiner/SILCombiner.h

@@ -220,8 +220,10 @@ class SILCombiner :
   // by this method.
   SILInstruction *eraseInstFromFunction(SILInstruction &I,
                                         SILBasicBlock::iterator &InstIter,
-                                        bool AddOperandsToWorklist = true) {
-    Worklist.eraseInstFromFunction(I, InstIter, AddOperandsToWorklist);
+                                        bool AddOperandsToWorklist = true,
+                                        bool salvageDebugInfo = true) {
+    Worklist.eraseInstFromFunction(I, InstIter, AddOperandsToWorklist,
+                                   salvageDebugInfo);
     MadeChange = true;
     // Dummy return, so the caller doesn't need to explicitly return nullptr.
     return nullptr;
@@ -232,9 +234,10 @@ class SILCombiner :
   void eraseInstIncludingUsers(SILInstruction *inst);
 
   SILInstruction *eraseInstFromFunction(SILInstruction &I,
-                                        bool AddOperandsToWorklist = true) {
+                                        bool AddOperandsToWorklist = true,
+                                        bool salvageDebugInfo = true) {
     SILBasicBlock::iterator nullIter;
-    return eraseInstFromFunction(I, nullIter, AddOperandsToWorklist);
+    return eraseInstFromFunction(I, nullIter, AddOperandsToWorklist, salvageDebugInfo);
   }
 
   void addInitialGroup(ArrayRef<SILInstruction *> List) {

test/SILOptimizer/globalopt_resilience.swift

@@ -37,13 +37,9 @@ public func cannotConvertToValueUse() {
 }
 
 // CHECK-LABEL: sil @$s4test23cannotConvertToValueUseyyF : $@convention(thin) () -> ()
-// CHECK: [[INT:%.*]] = integer_literal $Builtin.Int{{32|64}}, 27
-// CHECK: [[S1:%.*]] = struct $Int ([[INT]] : $Builtin.Int{{32|64}})
-// CHECK: [[S2:%.*]] = struct $ResilientStruct ([[S1]] : $Int)
-// CHECK: [[TMP:%.*]] = alloc_stack $ResilientStruct
-// CHECK: store [[S2]] to [[TMP]] : $*ResilientStruct
+// CHECK: [[GA:%.*]] = global_addr @$s4test15ResilientStructV9staticValACvpZ
 // CHECK: [[METHOD:%.*]] = function_ref @$s4test15ResilientStructV6methodyyF : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
-// CHECK: apply [[METHOD]]([[TMP]]) : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
+// CHECK: apply [[METHOD]]([[GA]]) : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
 // CHECK: [[RESULT:%.*]] = tuple ()
 // CHECK: return [[RESULT]] : $()