Merge pull request #63568 from eeckstein/fix-compute-side-effects

eeckstein · web-flow · commit 1aa400cc73b3 · 2023-02-12T11:25:19.000+01:00
Fix two bugs in side effect computation
diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ComputeSideEffects.swift b/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)
@@ -262,7 +299,7 @@ private struct CollectedEffects {
           if let calleePath = calleeEffect.destroy { addEffects(.destroy, to: argument, fromInitialPath: calleePath) }
         } else {
           let convention = callee.getArgumentConvention(for: calleeArgIdx)
-          let wholeArgument = argument.at(SmallProjectionPath(.anything))
+          let wholeArgument = argument.at(defaultPath(for: argument))
           let calleeEffects = callee.getSideEffects(forArgument: wholeArgument,
                                                     atIndex: calleeArgIdx,
                                                     withConvention: convention)
@@ -272,27 +309,16 @@ 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
   /// to the corrseponding `argumentEffects`. Otherwise they are added to the `global` effects.
+  private mutating func addEffects(_ effects: SideEffects.GlobalEffects, to value: Value) {
+    addEffects(effects, to: value, fromInitialPath: defaultPath(for: value))
+  }
+
   private mutating func addEffects(_ effects: SideEffects.GlobalEffects, to value: Value,
-                                   fromInitialPath: SmallProjectionPath? = nil) {
+                                   fromInitialPath: SmallProjectionPath) {
 
     /// Collects the (non-address) roots of a value.
     struct GetRootsWalker : ValueUseDefWalker {
@@ -324,10 +350,7 @@ private struct CollectedEffects {
 
     var findRoots = GetRootsWalker(context)
     if value.type.isAddress {
-      // If there is no initial path provided, select all value fields.
-      let path = fromInitialPath ?? SmallProjectionPath(.anyValueFields)
-
-      let accessPath = value.getAccessPath(fromInitialPath: path)
+      let accessPath = value.getAccessPath(fromInitialPath: fromInitialPath)
       switch accessPath.base {
         case .stack:
           // We don't care about read and writes from/to stack locations (because they are
@@ -347,16 +370,7 @@ private struct CollectedEffects {
           }
       }
     } else {
-      // Handle non-address `value`s which are projections from a direct arguments.
-      let path: SmallProjectionPath
-      if let fromInitialPath = fromInitialPath {
-        path = fromInitialPath
-      } else if value.type.isClass {
-        path = SmallProjectionPath(.anyValueFields).push(.anyClassField)
-      } else {
-        path = SmallProjectionPath(.anyValueFields).push(.anyClassField).push(.anyValueFields)
-      }
-      _ = findRoots.walkUp(value: value, path: path)
+      _ = findRoots.walkUp(value: value, path: fromInitialPath)
     }
     // Because of phi-arguments, a single (non-address) `value` can come from multiple arguments.
     while let (arg, path) = findRoots.roots.pop() {
@@ -369,6 +383,16 @@ private struct CollectedEffects {
   }
 }
 
+private func defaultPath(for value: Value) -> SmallProjectionPath {
+  if value.type.isAddress {
+    return SmallProjectionPath(.anyValueFields)
+  }
+  if value.type.isClass {
+    return SmallProjectionPath(.anyValueFields).push(.anyClassField)
+  }
+  return SmallProjectionPath(.anyValueFields).push(.anyClassField).push(.anyValueFields)
+}
+
 /// Checks if an argument escapes to some unknown user.
 private struct ArgumentEscapingWalker : ValueDefUseWalker, AddressDefUseWalker {
   var walkDownCache = WalkerCache<UnusedWalkingPath>()
diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift
@@ -382,7 +382,7 @@ fileprivate struct EscapeWalker<V: EscapeVisitor> : ValueDefUseWalker,
         
         // Class references, which are directly located in the array elements cannot escape,
         // because those are passed as `self` to their deinits - and `self` cannot escape in a deinit.
-        if path.projectionPath.hasNoClassProjection {
+        if !path.projectionPath.mayHaveClassProjection {
           return .continueWalk
         }
         return isEscaping
@@ -818,7 +818,7 @@ fileprivate struct EscapeWalker<V: EscapeVisitor> : ValueDefUseWalker,
   private func followLoads(at path: SmallProjectionPath) -> Bool {
     return visitor.followLoads ||
            // When part of a class field we have to follow loads.
-           !path.hasNoClassProjection
+           path.mayHaveClassProjection
   }
 
   private func pathForArgumentEscapeChecking(_ path: SmallProjectionPath) -> SmallProjectionPath {
diff --git a/SwiftCompilerSources/Sources/SIL/Effects.swift b/SwiftCompilerSources/Sources/SIL/Effects.swift
@@ -88,9 +88,41 @@ extension Function {
   public func getSideEffects(forArgument argument: ProjectedValue,
                              atIndex argumentIndex: Int,
                              withConvention convention: ArgumentConvention) -> SideEffects.GlobalEffects {
+    var result = SideEffects.GlobalEffects()
+
+    // Effects are only defined for operations which don't involve a load.
+    // In case the argument's path involves a load we need to return the global effects.
+    if argument.value.type.isAddress {
+      // Indirect arguments:
+      if argument.path.mayHaveClassProjection {
+        // For example:
+        //   bb0(%0: $*C):
+        //     %1 = load %0               // the involved load
+        //     %2 = ref_element_addr %1   // class projection
+        return getSideEffects()
+      }
+    } else {
+      // Direct arguments:
+      if argument.path.mayHaveTwoClassProjections {
+        // For example:
+        //   bb0(%0: $C):
+        //     %1 = ref_element_addr %1   // first class projection
+        //     %2 = load %1               // the involved load
+        //     %3 = ref_element_addr %2   // second class projection
+        return getSideEffects()
+
+      } else if argument.path.mayHaveClassProjection {
+        // For example:
+        //   bb0(%0: $C):
+        //     %1 = ref_element_addr %1   // class projection
+        //     %2 = load [take] %1        // the involved load
+        //     destroy_value %2
+        result.ownership = getSideEffects().ownership
+      }
+    }
+
     if let sideEffects = effects.sideEffects {
       /// There are computed side effects.
-      var result = SideEffects.GlobalEffects()
       let argEffect = sideEffects.getArgumentEffects(for: argumentIndex)
       if let effectPath = argEffect.read, effectPath.mayOverlap(with: argument.path) {
         result.memory.read = true
@@ -499,7 +531,7 @@ public struct SideEffects : CustomStringConvertible, NoReflectionChildren {
       case .directGuaranteed:
         // Note that `directGuaranteed` still has a "destroy" effect, because an object stored in
         // a class property could be destroyed.
-        if argument.path.hasNoClassProjection {
+        if !argument.path.mayHaveClassProjection {
           result.ownership.destroy = false
         }
         fallthrough
diff --git a/SwiftCompilerSources/Sources/SIL/SmallProjectionPath.swift b/SwiftCompilerSources/Sources/SIL/SmallProjectionPath.swift
@@ -244,15 +244,6 @@ public struct SmallProjectionPath : Hashable, CustomStringConvertible, NoReflect
     }
   }
 
-  /// Returns true if the path does not have any class projections.
-  /// For example:
-  /// returns true for `v**`
-  /// returns false for `c0`
-  /// returns false for `**` (because '**' can have any number of class projections)
-  public var hasNoClassProjection: Bool {
-    return matches(pattern: Self(.anyValueFields))
-  }
-
   /// Returns true if the path has at least one class projection.
   /// For example:
   /// returns false for `v**`
@@ -268,6 +259,27 @@ public struct SmallProjectionPath : Hashable, CustomStringConvertible, NoReflect
     }
   }
 
+  /// Returns true if the path may have a class projection.
+  /// For example:
+  /// returns false for `v**`
+  /// returns true for `c0`
+  /// returns true for `**` (because '**' can have any number of class projections)
+  public var mayHaveClassProjection: Bool {
+    return !matches(pattern: Self(.anyValueFields))
+  }
+
+  /// Returns true if the path may have a class projection.
+  /// For example:
+  /// returns false for `v**`
+  /// returns false for `c0`
+  /// returns true for `c0.c1`
+  /// returns true for `c0.**` (because '**' can have any number of class projections)
+  /// returns true for `**` (because '**' can have any number of class projections)
+  public var mayHaveTwoClassProjections: Bool {
+    return !matches(pattern: Self(.anyValueFields)) &&
+           !matches(pattern: Self(.anyValueFields).push(.anyClassField).push(.anyValueFields))
+  }
+
   /// Pops all value field components from the beginning of the path.
   /// For example:
   ///    `s0.e2.3.c4.s1` -> `c4.s1`
@@ -679,17 +691,24 @@ extension SmallProjectionPath {
     }
 
     func predicates() {
-      testPredicate("v**", \.hasNoClassProjection, expect: true)
-      testPredicate("c0",  \.hasNoClassProjection, expect: false)
-      testPredicate("1",   \.hasNoClassProjection, expect: true)
-      testPredicate("**",  \.hasNoClassProjection, expect: false)
-
       testPredicate("v**",           \.hasClassProjection, expect: false)
       testPredicate("v**.c0.s1.v**", \.hasClassProjection, expect: true)
       testPredicate("c0.**",         \.hasClassProjection, expect: true)
       testPredicate("c0.c1",         \.hasClassProjection, expect: true)
       testPredicate("ct",            \.hasClassProjection, expect: true)
       testPredicate("s0",            \.hasClassProjection, expect: false)
+
+      testPredicate("v**", \.mayHaveClassProjection, expect: false)
+      testPredicate("c0",  \.mayHaveClassProjection, expect: true)
+      testPredicate("1",   \.mayHaveClassProjection, expect: false)
+      testPredicate("**",  \.mayHaveClassProjection, expect: true)
+
+      testPredicate("v**", \.mayHaveTwoClassProjections, expect: false)
+      testPredicate("c0",  \.mayHaveTwoClassProjections, expect: false)
+      testPredicate("**",  \.mayHaveTwoClassProjections, expect: true)
+      testPredicate("v**.c*.s2.1.c0",   \.mayHaveTwoClassProjections, expect: true)
+      testPredicate("c*.s2.1.c0.v**",   \.mayHaveTwoClassProjections, expect: true)
+      testPredicate("v**.c*.**",        \.mayHaveTwoClassProjections, expect: true)
     }
 
     func testPredicate(_ pathStr: String, _ property: (SmallProjectionPath) -> Bool, expect: Bool) {
diff --git a/test/SILOptimizer/mem-behavior.sil b/test/SILOptimizer/mem-behavior.sil
@@ -16,7 +16,7 @@ class X {
 class Derived : X { }
 
 class C {
-  @_hasStorage @_hasInitialValue final let prop: Builtin.Int32 { get }
+  @_hasStorage @_hasInitialValue final var prop: Builtin.Int32 { get }
 }
 
 class Parent {
@@ -950,3 +950,33 @@ bb0 (%0 : $*C):
   return %3 : $()
 }
 
+sil @do_store : $@convention(thin) (@guaranteed Parent) -> () {
+[%0: noescape **, read c0.v**]
+[global: write]
+}
+
+// CHECK-LABEL: @test_double_class_indirection
+// CHECK:       PAIR #4.
+// CHECK-NEXT:      %8 = apply %7(%1) : $@convention(thin) (@guaranteed Parent) -> ()
+// CHECK-NEXT:      %3 = ref_element_addr %1 : $Parent, #Parent.child
+// CHECK-NEXT:    r=1,w=0
+// CHECK-NEXT:  PAIR #5.
+// CHECK-NEXT:      %8 = apply %7(%1) : $@convention(thin) (@guaranteed Parent) -> ()
+// CHECK-NEXT:      %5 = ref_element_addr %2 : $C, #C.prop
+// CHECK-NEXT:    r=1,w=1
+sil @test_double_class_indirection : $@convention(thin) (Builtin.Int32) -> () {
+bb0(%0 : $Builtin.Int32):
+  %1 = alloc_ref $Parent
+  %2 = alloc_ref $C
+  %3 = ref_element_addr %1 : $Parent, #Parent.child
+  store %2 to %3 : $*C
+  %5 = ref_element_addr %2 : $C, #C.prop
+  store %0 to %5 : $*Builtin.Int32
+
+  %f = function_ref @do_store : $@convention(thin) (@guaranteed Parent) -> ()
+  %a = apply %f(%1) : $@convention(thin) (@guaranteed Parent) -> ()
+
+  %r = tuple ()
+  return %r : $()
+}
+
diff --git a/test/SILOptimizer/side_effects.sil b/test/SILOptimizer/side_effects.sil
