ComputeSideEffects: fix wrong side effect computation of releases/destroys

Only global side effects of the destructor were considered, but side effects weren't attributed to the released value.

rdar://105237110

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ComputeSideEffects.swift

@@ -95,15 +95,10 @@ private struct CollectedEffects {
       addEffects(.copy, to: inst.operands[0].value, fromInitialPath: SmallProjectionPath(.anyValueFields))
 
     case is DestroyValueInst, is ReleaseValueInst, is StrongReleaseInst:
-      addDestroyEffects(of: inst.operands[0].value)
+      addDestroyEffects(ofValue: inst.operands[0].value)
 
     case let da as DestroyAddrInst:
-      // A destroy_addr also involves a read from the address. It's equivalent to a `%x = load [take]` and `destroy_value %x`.
-      addEffects(.read, to: da.operand)
-      // Conceptually, it's also a write, because the stored value is not available anymore after the destroy
-      addEffects(.write, to: da.operand)
-
-      addDestroyEffects(of: da.operand)
+      addDestroyEffects(ofAddress: da.operand)
 
     case let copy as CopyAddrInst:
       addEffects(.read, to: copy.source)
@@ -113,17 +108,13 @@ private struct CollectedEffects {
         addEffects(.copy, to: copy.source)
       }
       if !copy.isInitializationOfDest {
-        // Like for destroy_addr, the destroy also involves a read.
-        addEffects(.read, to: copy.destination)
-        addDestroyEffects(of: copy.destination)
+        addDestroyEffects(ofAddress: copy.destination)
       }
 
     case let store as StoreInst:
       addEffects(.write, to: store.destination)
       if store.destinationOwnership == .assign {
-        // Like for destroy_addr, the destroy also involves a read.
-        addEffects(.read, to: store.destination)
-        addDestroyEffects(of: store.destination)
+        addDestroyEffects(ofAddress: store.destination)
       }
 
     case let store as StoreWeakInst:
@@ -137,9 +128,9 @@ private struct CollectedEffects {
       addEffects(.read, to: addr)
 
     case let apply as FullApplySite:
-      let calleeValue = apply.callee
       if apply.callee.type.isCalleeConsumedFunction {
-        addDestroyEffects(of: calleeValue)
+        addEffects(.destroy, to: apply.callee)
+        globalEffects = .worstEffects
       }
       handleApply(apply)
       checkedIfDeinitBarrier = true
@@ -230,11 +221,58 @@ private struct CollectedEffects {
 
   private mutating func handleApply(_ apply: ApplySite) {
     let callees = calleeAnalysis.getCallees(callee: apply.callee)
+    let args = apply.arguments.enumerated().lazy.map {
+      (calleeArgumentIndex: apply.calleeArgIndex(callerArgIndex: $0.0),
+       callerArgument: $0.1)
+    }
+    addEffects(ofFunctions: callees, withArguments: args)
+  }
+
+  private mutating func addDestroyEffects(ofValue value: Value) {
+    // First thing: add the destroy effect itself.
+    addEffects(.destroy, to: value)
+
+    if value.type.isClass {
+      // Treat destroying a class value just like a call to it's destructor(s).
+      let destructors = calleeAnalysis.getDestructors(of: value.type)
+      let theSelfArgument = CollectionOfOne((calleeArgumentIndex: 0, callerArgument: value))
+      addEffects(ofFunctions: destructors, withArguments: theSelfArgument)
+    } else {
+      // TODO: dig into the type and check for destructors of individual class fields
+      addEffects(.worstEffects, to: value)
+      globalEffects = .worstEffects
+    }
+  }
+
+  private mutating func addDestroyEffects(ofAddress address: Value) {
+    // First thing: add the destroy effect itself.
+    addEffects(.destroy, to: address)
+
+    // A destroy also involves a read from the address.
+    // E.g. a `destroy_addr` is equivalent to a `%x = load [take]` and `destroy_value %x`.
+    addEffects(.read, to: address)
+    // Conceptually, it's also a write, because the stored value is not available anymore after the destroy
+    addEffects(.write, to: address)
+
+    // Second: add all effects of (potential) destructors which might be called if the destroy deallocates an object.
+    // Note that we don't need to add any effects specific to the `address`, because the memory location is not
+    // affected by a destructor of the stored value (and effects don't include anything which is loaded from memory).
+    if let destructors = calleeAnalysis.getDestructors(of: address.type) {
+      for destructor in destructors {
+        globalEffects.merge(with: destructor.getSideEffects())
+      }
+    } else {
+      globalEffects = .worstEffects
+    }
+  }
 
+  private mutating func addEffects<Arguments: Sequence>(ofFunctions callees: FunctionArray?,
+                                                        withArguments arguments: Arguments)
+                                   where Arguments.Element == (calleeArgumentIndex: Int, callerArgument: Value) {
     guard let callees = callees else {
       // We don't know which function(s) are called.
       globalEffects = .worstEffects
-      for argument in apply.arguments {
+      for (_, argument) in arguments {
         addEffects(.worstEffects, to: argument)
       }
       return
@@ -249,8 +287,7 @@ private struct CollectedEffects {
       }
     }
 
-    for (argumentIdx, argument) in apply.arguments.enumerated() {
-      let calleeArgIdx = apply.calleeArgIndex(callerArgIndex: argumentIdx)
+    for (calleeArgIdx, argument) in arguments {
       for callee in callees {
         if let sideEffects = callee.effects.sideEffects {
           let calleeEffect = sideEffects.getArgumentEffects(for: calleeArgIdx)
@@ -272,21 +309,6 @@ private struct CollectedEffects {
     }
   }
 
-  private mutating func addDestroyEffects(of addressOrValue: Value) {
-    // First thing: add the destroy effect itself.
-    addEffects(.destroy, to: addressOrValue)
-
-    // Second: add all effects of (potential) destructors which might be called if the destroy
-    // deallocates an object.
-    if let destructors = calleeAnalysis.getDestructors(of: addressOrValue.type) {
-      for destructor in destructors {
-        globalEffects.merge(with: destructor.getSideEffects())
-      }
-    } else {
-      globalEffects = .worstEffects
-    }
-  }
-
   /// Adds effects to a specific value.
   ///
   /// If the value comes from an argument (or mutliple arguments), then the effects are added

test/SILOptimizer/side_effects.sil

@@ -22,6 +22,13 @@ final class X {
 class Y {
 }
 
+final class Z {
+  @_hasStorage var a: Int32
+
+  init()
+  deinit
+}
+
 struct SP {
   var value: X
 }
@@ -73,6 +80,10 @@ sil @$s4test1XCfD : $@convention(method) (@owned X) -> () {
 [global: copy, write]
 }
 
+sil @$s4test1ZCfD : $@convention(method) (@owned Z) -> () {
+[%0: write c0.v**]
+}
+
 // CHECK-LABEL: sil @load_store_to_args
 // CHECK-NEXT:  [%0: read v**]
 // CHECK-NEXT:  [%1: write v**]
@@ -495,7 +506,7 @@ bb0(%0 : $*X, %1 : $*X):
 }
 
 // CHECK-LABEL: sil [ossa] @destroy_value_effects1
-// CHECK-NEXT:  [%0: destroy v**.c*.v**]
+// CHECK-NEXT:  [%0: read v**.c*.v**, write v**.c*.v**, copy v**.c*.v**, destroy v**.c*.v**]
 // CHECK-NEXT:  [global: read,write,copy,destroy,allocate,deinit_barrier]
 // CHECK-NEXT:  {{^[^[]}}
 sil [ossa] @destroy_value_effects1 : $@convention(thin) (@owned SP) -> () {
@@ -517,6 +528,17 @@ bb0(%0 : @owned $SP):
   return %2 : $()
 }
 
+// CHECK-LABEL: sil [ossa] @destroy_value_effects3
+// CHECK-NEXT:  [%0: write c0.v**, destroy c*.v**]
+// CHECK-NEXT:  [global: ]
+// CHECK-NEXT:  {{^[^[]}}
+sil [ossa] @destroy_value_effects3 : $@convention(thin) (@owned Z) -> () {
+bb0(%0 : @owned $Z):
+  destroy_value %0 : $Z
+  %2 = tuple ()
+  return %2 : $()
+}
+
 // CHECK-LABEL: sil [ossa] @destroy_addr_effects
 // CHECK-NEXT:  [%0: read v**, write v**, destroy v**]
 // CHECK-NEXT:  [global: write,copy]