Revert "TempRValueElimination: handle store instructions to the temporary stack location."

This reverts commit 5d24a26.

Author: hamishknight

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift

@@ -38,51 +38,18 @@ let tempRValueElimination = FunctionPass(name: "temp-rvalue-elimination") {
   (function: Function, context: FunctionPassContext) in
 
   for inst in function.instructions {
-    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
+    if let copyAddr = inst as? CopyAddrInst {
+      tryEliminate(copy: copyAddr, context)
     }
   }
 }
 
-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) {
+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)
+  }
 
   guard let (allocStack, lastUseOfAllocStack) = getRemovableAllocStackDestination(of: copy, context) else {
     return
@@ -94,7 +61,7 @@ private func tryEliminate(copy: CopyLikeInstruction, _ context: FunctionPassCont
 
   if needToInsertDestroy(copy: copy, lastUseOfAllocStack: lastUseOfAllocStack) {
     Builder.insert(after: lastUseOfAllocStack, context) { builder in
-      builder.createDestroyAddr(address: copy.sourceAddress)
+      builder.createDestroyAddr(address: copy.source)
     }
   }
 
@@ -109,25 +76,25 @@ private func tryEliminate(copy: CopyLikeInstruction, _ context: FunctionPassCont
         assert(cai == lastUseOfAllocStack && cai.source == allocStack)
         cai.set(isTakeOfSource: false, context)
       }
-      use.set(to: copy.sourceAddress, context)
+      use.set(to: copy.source, 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.sourceAddress, context);
+      use.set(to: copy.source, 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.sourceAddress, context);
+      use.set(to: copy.source, context);
     }
   }
   context.erase(instruction: allocStack)
-  context.erase(instructionIncludingAllUsers: copy.loadingInstruction)
+  context.erase(instruction: copy)
 }
 
 /// Checks if the `copy` is copying into an `alloc_stack` which is removable:
@@ -138,17 +105,17 @@ private func tryEliminate(copy: CopyLikeInstruction, _ context: FunctionPassCont
 ///   %lastUseOfAllocStack = load %allocStack
 /// ```
 private func getRemovableAllocStackDestination(
-  of copy: CopyLikeInstruction, _ context: FunctionPassContext
+  of copy: CopyAddrInst, _ context: FunctionPassContext
 ) -> (allocStack: AllocStackInst, lastUseOfAllocStack: Instruction)? {
   guard copy.isInitializationOfDestination,
-        let allocStack = copy.destinationAddress as? AllocStackInst
+        let allocStack = copy.destination 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.sourceAddress.accessBase.storageIsLexical {
+  if allocStack.isLexical && !copy.source.accessBase.storageIsLexical {
     return nil
   }
 
@@ -182,7 +149,7 @@ private func getRemovableAllocStackDestination(
   if copy.isTakeOfSource,
      lastUseOfAllocStack != copy,
      !(lastUseOfAllocStack is DestroyAddrInst),
-     lastUseOfAllocStack.mayWrite(toAddress: copy.sourceAddress, context.aliasAnalysis)
+     lastUseOfAllocStack.mayWrite(toAddress: copy.source, context.aliasAnalysis)
   {
     return nil
   }
@@ -201,7 +168,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: CopyLikeInstruction, lastUseOfAllocStack: Instruction) -> Bool {
+private func needToInsertDestroy(copy: CopyAddrInst, lastUseOfAllocStack: Instruction) -> Bool {
   if !copy.isTakeOfSource {
     return false
   }
@@ -210,13 +177,13 @@ private func needToInsertDestroy(copy: CopyLikeInstruction, lastUseOfAllocStack:
     return true
   case let cai as CopyAddrInst:
     if cai.isTakeOfSource {
-      assert(cai.source == copy.destinationAddress, "copy_addr must be not take a projected address")
+      assert(cai.source == copy.destination, "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.destinationAddress, "load must be not take a projected address")
+      assert(li.address == copy.destination, "load must be not take a projected address")
       return false
     }
     return true
@@ -280,9 +247,7 @@ 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: CopyLikeInstruction,
-                                _ context: FunctionPassContext) -> Bool
-{
+private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyAddrInst, _ context: FunctionPassContext) -> Bool {
   var endAccessToMove: EndAccessInst? = nil
 
   for inst in InstructionList(first: copy.next) {
@@ -292,7 +257,7 @@ private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyLikeInstr
       if endAccessToMove != nil {
         return false
       }
-      if context.aliasAnalysis.mayAlias(copy.sourceAddress, endAccess.beginAccess.address),
+      if context.aliasAnalysis.mayAlias(copy.source, 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`.
@@ -336,7 +301,7 @@ private func extendAccessScopes(beyond lastUse: Instruction, copy: CopyLikeInstr
 ///
 /// 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: CopyLikeInstruction,
+private func getLastUseWhileSourceIsNotModified(of copy: CopyAddrInst,
                                                 uses: InstructionSetWithCount,
                                                 _ context: FunctionPassContext) -> Instruction?
 {
@@ -348,7 +313,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyLikeInstruction,
 
   // 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.loadingInstruction.next) {
+  for inst in InstructionList(first: copy.next) {
     if uses.contains(inst) {
       numUsesFound += 1
     }
@@ -362,7 +327,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyLikeInstruction,
       // could occur _before_ the read of the `alloc_stack`.
       switch inst {
       case is FullApplySite, is YieldInst:
-        if inst.mayWrite(toAddress: copy.sourceAddress, aliasAnalysis) {
+        if inst.mayWrite(toAddress: copy.source, aliasAnalysis) {
           return nil
         }
         return inst
@@ -371,7 +336,7 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyLikeInstruction,
       }
     }
 
-    if inst.mayWrite(toAddress: copy.sourceAddress, aliasAnalysis) {
+    if inst.mayWrite(toAddress: copy.source, aliasAnalysis) {
       return nil
     }
   }
@@ -383,9 +348,9 @@ private func getLastUseWhileSourceIsNotModified(of copy: CopyLikeInstruction,
 /// Collects all uses of the `alloc_stack`.
 private struct UseCollector : AddressDefUseWalker {
   private(set) var uses: InstructionSetWithCount
-  private let copy: CopyLikeInstruction
+  private let copy: CopyAddrInst
 
-  init(copy: CopyLikeInstruction, _ context: FunctionPassContext) {
+  init(copy: CopyAddrInst, _ context: FunctionPassContext) {
     self.uses = InstructionSetWithCount(context)
     self.copy = copy
   }
@@ -502,7 +467,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.destinationAddress
+          load.address != copy.destination
       {
         return .abortWalk
       }
@@ -529,7 +494,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.destinationAddress {
+      if copyFromStack.isTakeOfSource && copyFromStack.source != copy.destination {
         return .abortWalk
       }
       uses.insert(copyFromStack)

test/SILOptimizer/globalopt_resilience.swift

@@ -37,9 +37,13 @@ public func cannotConvertToValueUse() {
 }
 
 // CHECK-LABEL: sil @$s4test23cannotConvertToValueUseyyF : $@convention(thin) () -> ()
-// CHECK: [[GA:%.*]] = global_addr @$s4test15ResilientStructV9staticValACvpZ
+// 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: [[METHOD:%.*]] = function_ref @$s4test15ResilientStructV6methodyyF : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
-// CHECK: apply [[METHOD]]([[GA]]) : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
+// CHECK: apply [[METHOD]]([[TMP]]) : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
 // CHECK: [[RESULT:%.*]] = tuple ()
 // CHECK: return [[RESULT]] : $()
 

test/SILOptimizer/inline_arrays.swift

@@ -189,6 +189,7 @@ public func usePrespecializedEntryPoints(wrapperStruct: ReferenceWrapperStruct,
 // OPT:   [[A1:%.*]] = unchecked_ref_cast [[P1]] : $SomeClass to $AnyObject
 // OPT:   [[R3:%.*]] = apply [[F1]]([[A1]]) : $@convention(thin) (@guaranteed AnyObject) -> @owned AnyObject
 // OPT:   store [[R3]] to [[R2]] : $*AnyObject
+// OPT:   [[A2:%.*]] = load [[R1]] : $*SomeClass
 // OPT:   [[F2:%.*]] = function_ref @$s22pre_specialize_layouts7consumeyyxlF0a20_specialized_module_C09SomeClassC_Ttg5 : $@convention(thin) () -> ()
 // OPT:   apply [[F2]]() : $@convention(thin) () -> ()
 // OPT:   dealloc_stack [[R1]] : $*SomeClass

test/SILOptimizer/pre_specialize_layouts.swift

@@ -421,12 +421,12 @@ struct PAEM : P4EM {
 // CHECK:   apply [[F]]([[V]])
 
 // CHECK-64-LABEL: sil hidden @$s1A2PAV4testyyF : $@convention(method) (PA) -> ()
-// CHECK-64:         [[V:%.*]] = integer_literal $Builtin.Int64, 5
-// CHECK-64-DAG:     [[I:%.*]] = struct $Int64 ([[V]] : $Builtin.Int64)
-// CHECK-64-DAG:     [[F:%.*]] = function_ref @$s1A4usePyyxAA1PRzlFs5Int64V_Tg5
-// CHECK-64:         apply [[F]]([[I]]) : $@convention(thin) (Int64) -> ()
-// CHECK-64:         apply [[F]]([[I]]) : $@convention(thin) (Int64) -> ()
-// CHECK-64:       } // end sil function '$s1A2PAV4testyyF'
+// CHECK-64:   [[V:%.*]] = integer_literal $Builtin.Int64, 5
+// CHECK-64:   [[I:%.*]] = struct $Int64 ([[V]] : $Builtin.Int64)
+// CHECK-64:   [[F:%.*]] = function_ref @$s1A4usePyyxAA1PRzlFs5Int64V_Tg5
+// CHECK-64:   apply [[F]]([[I]]) : $@convention(thin) (Int64) -> ()
+// CHECK-64:   apply [[F]]([[I]]) : $@convention(thin) (Int64) -> ()
+
 @inline(never)
 func testIt<T>(cl: (Int64) throws -> T) {
  do {

test/SILOptimizer/specialize_opaque_type_archetypes.swift

@@ -827,18 +827,3 @@ bb0:
   return %12
 }
 
-// CHECK-LABEL: sil @store_to_temp :
-// CHECK:         %1 = load %0
-// CHECK-NEXT:    return %1
-// CHECK-LABEL: } // end sil function 'store_to_temp'
-sil @store_to_temp : $@convention(thin) (@in_guaranteed Klass) -> @owned Klass {
-bb0(%0 : $*Klass):
-  %1 = load %0
-  %2 = alloc_stack $Klass
-  store %1 to %2
-  %4 = load %2
-  destroy_addr %2
-  dealloc_stack %2
-  return %4
-}
-