DeadStoreElimination: some refactoring and improvements

eeckstein · eeckstein · commit 9cb83c33ebdd · 2023-07-11T22:33:03.000+02:00
Addresses review feedback of swiftlang#67122
diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DeadStoreElimination.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DeadStoreElimination.swift
@@ -14,7 +14,7 @@ import SIL
 
 /// Eliminates dead store instructions.
 ///
-/// A store is dead if after the store has occurred:
+/// A store is dead if, after the store has occurred:
 ///
 /// 1. The value in memory is not read until the memory object is deallocated:
 ///
@@ -47,6 +47,9 @@ import SIL
 ///     ...              // no reads from %1
 ///     store %7 to %3
 ///
+/// The algorithm is a data flow analysis which starts at the original store and searches
+/// for successive stores by following the control flow in forward direction.
+///
 let deadStoreElimination = FunctionPass(name: "dead-store-elimination") {
   (function: Function, context: FunctionPassContext) in
 
@@ -85,7 +88,7 @@ private func tryEliminate(store: StoreInst, _ context: FunctionPassContext) {
         case .dead:
           // The new individual stores are inserted right after the current store and
           // will be optimized in the following loop iterations.
-          store.trySplit(context)
+          store.split(context)
       }
   }
 }
@@ -111,18 +114,24 @@ private extension StoreInst {
   }
 
   func isDead(at accessPath: AccessPath, _ context: FunctionPassContext) -> DataflowResult {
-    var worklist = InstructionWorklist(context)
-    defer { worklist.deinitialize() }
+    var scanner = InstructionScanner(storePath: accessPath, storeAddress: self.destination, context.aliasAnalysis)
+    let storageDefBlock = accessPath.base.reference?.referenceRoot.parentBlock
 
-    worklist.pushIfNotVisited(self.next!)
+    switch scanner.scan(instructions: InstructionList(first: self.next)) {
+    case .dead:
+      return .dead
 
-    let storageDefBlock = accessPath.base.reference?.referenceRoot.parentBlock
-    var scanner = InstructionScanner(storePath: accessPath, storeAddress: self.destination, context.aliasAnalysis)
+    case .alive:
+      return DataflowResult(aliveWith: scanner.potentiallyDeadSubpath)
+
+    case .transparent:
+      // Continue with iterative data flow analysis starting at the block's successors.
+      var worklist = BasicBlockWorklist(context)
+      defer { worklist.deinitialize() }
+      worklist.pushIfNotVisited(contentsOf: self.parentBlock.successors)
+
+      while let block = worklist.pop() {
 
-    while let startInstInBlock = worklist.pop() {
-      let block = startInstInBlock.parentBlock
-      switch scanner.scan(instructions: InstructionList(first: startInstInBlock)) {
-      case .transparent:
         // Abort if we find the storage definition of the access in case of a loop, e.g.
         //
         //   bb1:
@@ -133,39 +142,41 @@ private extension StoreInst {
         //
         // The storage root is different in each loop iteration. Therefore the store of a
         // successive loop iteration does not overwrite the store of the previous iteration.
-        if let storageDefBlock = storageDefBlock,
-           block.successors.contains(storageDefBlock) {
+        if let storageDefBlock = storageDefBlock, block == storageDefBlock {
+          return DataflowResult(aliveWith: scanner.potentiallyDeadSubpath)
+        }
+        switch scanner.scan(instructions: block.instructions) {
+        case .transparent:
+          worklist.pushIfNotVisited(contentsOf: block.successors)
+        case .dead:
+          break
+        case .alive:
           return DataflowResult(aliveWith: scanner.potentiallyDeadSubpath)
         }
-        worklist.pushIfNotVisited(contentsOf: block.successors.lazy.map { $0.instructions.first! })
-      case .dead:
-        break
-      case .alive:
-        return DataflowResult(aliveWith: scanner.potentiallyDeadSubpath)
       }
+      return .dead
     }
-    return .dead
   }
 
-  func trySplit(_ context: FunctionPassContext) {
+  func split(_ context: FunctionPassContext) {
+    let builder = Builder(after: self, context)
     let type = source.type
     if type.isStruct {
-      let builder = Builder(after: self, context)
       for idx in 0..<type.getNominalFields(in: parentFunction).count {
         let srcField = builder.createStructExtract(struct: source, fieldIndex: idx)
         let destFieldAddr = builder.createStructElementAddr(structAddress: destination, fieldIndex: idx)
         builder.createStore(source: srcField, destination: destFieldAddr, ownership: storeOwnership)
       }
-      context.erase(instruction: self)
     } else if type.isTuple {
-      let builder = Builder(after: self, context)
       for idx in 0..<type.tupleElements.count {
         let srcField = builder.createTupleExtract(tuple: source, elementIndex: idx)
         let destFieldAddr = builder.createTupleElementAddr(tupleAddress: destination, elementIndex: idx)
         builder.createStore(source: srcField, destination: destFieldAddr, ownership: storeOwnership)
       }
-      context.erase(instruction: self)
+    } else {
+      fatalError("a materializable projection path should only contain struct and tuple projections")
     }
+    context.erase(instruction: self)
   }
 
   var hasValidOwnershipForDeadStoreElimination: Bool {
@@ -181,11 +192,11 @@ private extension StoreInst {
 }
 
 private struct InstructionScanner {
-  let storePath: AccessPath
-  let storeAddress: Value
-  let aliasAnalysis: AliasAnalysis
+  private let storePath: AccessPath
+  private let storeAddress: Value
+  private let aliasAnalysis: AliasAnalysis
 
-  var potentiallyDeadSubpath: AccessPath? = nil
+  private(set) var potentiallyDeadSubpath: AccessPath? = nil
 
   // Avoid quadratic complexity by limiting the number of visited instructions for each store.
   // The limit of 1000 instructions is not reached by far in "real-world" functions.
@@ -208,14 +219,16 @@ private struct InstructionScanner {
       switch inst {
       case let successiveStore as StoreInst:
         let successivePath = successiveStore.destination.accessPath
-        if successivePath.isEqualOrOverlaps(storePath) {
+        if successivePath.isEqualOrContains(storePath) {
           return .dead
         }
-        if storePath.isEqualOrOverlaps(successivePath),
-           potentiallyDeadSubpath == nil {
+        if potentiallyDeadSubpath == nil,
+           storePath.getMaterializableProjection(to: successivePath) != nil {
           // Storing to a sub-field of the original store doesn't make the original store dead.
           // But when we split the original store, then one of the new individual stores might be
           // overwritten by this store.
+          // Requiring that the projection to the partial store path is materializable guarantees
+          // that we can split the store.
           potentiallyDeadSubpath = successivePath
         }
       case is DeallocRefInst, is DeallocStackRefInst, is DeallocBoxInst:
@@ -241,6 +254,8 @@ private struct InstructionScanner {
         if inst.mayRead(fromAddress: storeAddress, aliasAnalysis) {
           return .alive
         }
+        // TODO: We might detect that this is a partial read of the original store which potentially
+        //       enables partial dead store elimination.
       }
     }
     return .transparent
diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/AccessUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/AccessUtils.swift
@@ -162,7 +162,11 @@ enum AccessBase : CustomStringConvertible, Hashable {
     }
   }
 
-  // Returns true if it's guaranteed that this access has the same base address as the `other` access.
+  /// Returns true if it's guaranteed that this access has the same base address as the `other` access.
+  ///
+  /// `isEqual` abstracts away the projection instructions that are included as part of the AccessBase:
+  /// multiple `project_box` and `ref_element_addr` instructions are equivalent bases as long as they
+  /// refer to the same variable or class property.
   func isEqual(to other: AccessBase) -> Bool {
     switch (self, other) {
     case (.box(let pb1), .box(let pb2)):
@@ -275,11 +279,52 @@ struct AccessPath : CustomStringConvertible {
     return false
   }
 
-  func isEqualOrOverlaps(_ other: AccessPath) -> Bool {
-    return base.isEqual(to: other.base) &&
-           // Note: an access with a smaller path overlaps an access with larger path, e.g.
-           //       `s0` overlaps `s0.s1`
-           projectionPath.isSubPath(of: other.projectionPath)
+  /// Returns true if this access addresses the same memory location as `other` or if `other`
+  /// is a sub-field of this access.
+
+  /// Note that this access _contains_ `other` if `other` has a _larger_ projection path than this acccess.
+  /// For example:
+  ///   `%value.s0` contains `%value.s0.s1`
+  func isEqualOrContains(_ other: AccessPath) -> Bool {
+    return getProjection(to: other) != nil
+  }
+
+  /// Returns the projection path to `other` if this access path is equal or contains `other`.
+  ///
+  /// For example,
+  ///   `%value.s0`.getProjection(to: `%value.s0.s1`)
+  /// yields
+  ///   `s1`
+  func getProjection(to other: AccessPath) -> SmallProjectionPath? {
+    if !base.isEqual(to: other.base) {
+      return nil
+    }
+    return projectionPath.subtract(from: other.projectionPath)
+  }
+
+  /// Like `getProjection`, but also requires that the resulting projection path is materializable.
+  func getMaterializableProjection(to other: AccessPath) -> SmallProjectionPath? {
+    if let projectionPath = getProjection(to: other),
+       projectionPath.isMaterializable {
+      return projectionPath
+    }
+    return nil
+  }
+}
+
+private extension SmallProjectionPath {
+  /// Returns true if the path only contains projections which can be materialized as
+  /// SIL struct or tuple projection instructions - for values or addresses.
+  var isMaterializable: Bool {
+    let (kind, _, subPath) = pop()
+    switch kind {
+    case .root:
+      return true
+    case .structField, .tupleField:
+      return subPath.isMaterializable
+    default:
+      return false
+    }
   }
 }
 
diff --git a/SwiftCompilerSources/Sources/SIL/SmallProjectionPath.swift b/SwiftCompilerSources/Sources/SIL/SmallProjectionPath.swift
@@ -73,7 +73,7 @@ public struct SmallProjectionPath : Hashable, CustomStringConvertible, NoReflect
     case enumCase       = 0x3 // A concrete enum case (with payload): syntax e.g. `e4'
     case classField     = 0x4 // A concrete class field: syntax e.g. `c1`
     case indexedElement = 0x5 // A constant offset into an array of elements: syntax e.g. 'i2'
-                              // The index must not be 0 and there must not be two successive element indices in the path.
+                              // The index must be greater than 0 and there must not be two successive element indices in the path.
 
     // "Large" kinds: starting from here the low 3 bits must be 1.
     // This and all following kinds (we'll add in the future) cannot have a field index.
@@ -200,12 +200,18 @@ public struct SmallProjectionPath : Hashable, CustomStringConvertible, NoReflect
   public func push(_ kind: FieldKind, index: Int = 0) -> SmallProjectionPath {
     assert(kind != .anything || bytes == 0, "'anything' only allowed in last path component")
     if (kind.isIndexedElement) {
-      if kind == .indexedElement && index == 0 {
-        // Ignore zero indices
-        return self
+      let (k, i, numBits) = top
+      if kind == .indexedElement {
+        if index == 0 {
+          // Ignore zero indices
+          return self
+        }
+        if k == .indexedElement {
+          // "Merge" two constant successive indexed elements
+          return pop(numBits: numBits).push(.indexedElement, index: index + i)
+        }
       }
-      // "Merge" two successive indexed elements
-      let (k, _, numBits) = top
+      // "Merge" two successive indexed elements which doesn't have a constant result
       if (k.isIndexedElement) {
         return pop(numBits: numBits).push(.anyIndexedElement)
       }
@@ -474,26 +480,26 @@ public struct SmallProjectionPath : Hashable, CustomStringConvertible, NoReflect
     return false
   }
 
-  /// Return true if this path is a sub-path of `rhs` or is equivalent to `rhs`.
+  /// Subtracts this path from a larger path if this path is a prefix of the other path.
   ///
   /// For example:
-  ///   `s0` is a sub-path of `s0.s1`
-  ///   `s0` is not a sub-path of `s1`
-  ///   `s0.s1` is a sub-path of `s0.s1`
-  ///   `i*.s1` is not a sub-path of `i*.s1` because the actual field is unknown on both sides
-  public func isSubPath(of rhs: SmallProjectionPath) -> Bool {
+  ///   subtracting `s0` from `s0.s1` yields `s1`
+  ///   subtracting `s0` from `s1` yields nil, because `s0` is not a prefix of `s1`
+  ///   subtracting `s0.s1` from `s0.s1` yields an empty path
+  ///   subtracting `i*.s1` from `i*.s1` yields nil, because the actual index is unknown on both sides
+  public func subtract(from rhs: SmallProjectionPath) -> SmallProjectionPath? {
     let (lhsKind, lhsIdx, lhsBits) = top
     switch lhsKind {
     case .root:
-      return true
+      return rhs
     case .classField, .tailElements, .structField, .tupleField, .enumCase, .existential, .indexedElement:
       let (rhsKind, rhsIdx, rhsBits) = rhs.top
       if lhsKind == rhsKind && lhsIdx == rhsIdx {
-        return pop(numBits: lhsBits).isSubPath(of: rhs.pop(numBits: rhsBits))
+        return pop(numBits: lhsBits).subtract(from: rhs.pop(numBits: rhsBits))
       }
-      return false
+      return nil
     case .anything, .anyValueFields, .anyClassField, .anyIndexedElement:
-      return false
+      return nil
     }
   }
 }
@@ -632,9 +638,9 @@ extension SmallProjectionPath {
     basicPushPop()
     parsing()
     merging()
+    subtracting()
     matching()
     overlapping()
-    subPathTesting()
     predicates()
     path2path()
   
@@ -652,9 +658,15 @@ extension SmallProjectionPath {
       assert(p5.pop().path.isEmpty)
       let p6 = SmallProjectionPath(.indexedElement, index: 1).push(.indexedElement, index: 2)
       let (k6, i6, p7) = p6.pop()
-      assert(k6 == .anyIndexedElement && i6 == 0 && p7.isEmpty)
+      assert(k6 == .indexedElement && i6 == 3 && p7.isEmpty)
       let p8 = SmallProjectionPath(.indexedElement, index: 0)
       assert(p8.isEmpty)
+      let p9 = SmallProjectionPath(.indexedElement, index: 1).push(.anyIndexedElement)
+      let (k9, i9, p10) = p9.pop()
+      assert(k9 == .anyIndexedElement && i9 == 0 && p10.isEmpty)
+      let p11 = SmallProjectionPath(.anyIndexedElement).push(.indexedElement, index: 1)
+      let (k11, i11, p12) = p11.pop()
+      assert(k11 == .anyIndexedElement && i11 == 0 && p12.isEmpty)
     }
     
     func parsing() {
@@ -727,6 +739,35 @@ extension SmallProjectionPath {
       assert(result2 == expect)
     }
    
+    func subtracting() {
+      testSubtract("s0",           "s0.s1",        expect: "s1")
+      testSubtract("s0",           "s1",           expect: nil)
+      testSubtract("s0.s1",        "s0.s1",        expect: "")
+      testSubtract("i*.s1",        "i*.s1",        expect: nil)
+      testSubtract("ct.s1.0.i3.x", "ct.s1.0.i3.x", expect: "")
+      testSubtract("c0.s1.0.i3",   "c0.s1.0.i3.x", expect: "x")
+      testSubtract("s1.0.i3.x",    "s1.0.i3",      expect: nil)
+      testSubtract("v**.s1",       "v**.s1",       expect: nil)
+      testSubtract("i*",           "i*",           expect: nil)
+    }
+
+    func testSubtract(_ lhsStr: String, _ rhsStr: String, expect expectStr: String?) {
+      var lhsParser = StringParser(lhsStr)
+      let lhs = try! lhsParser.parseProjectionPathFromSIL()
+      var rhsParser = StringParser(rhsStr)
+      let rhs = try! rhsParser.parseProjectionPathFromSIL()
+
+      let result = lhs.subtract(from: rhs)
+
+      if let expectStr = expectStr {
+        var expectParser = StringParser(expectStr)
+        let expect = try! expectParser.parseProjectionPathFromSIL()
+        assert(result! == expect)
+      } else {
+        assert(result == nil)
+      }
+    }
+
     func matching() {
       testMatch("ct", "c*",  expect: true)
       testMatch("c1", "c*",  expect: true)
@@ -799,27 +840,6 @@ extension SmallProjectionPath {
       assert(reversedResult == expect)
     }
 
-    func subPathTesting() {
-      testSubPath("s0", "s0.s1",                  expect: true)
-      testSubPath("s0", "s1",                     expect: false)
-      testSubPath("s0.s1", "s0.s1",               expect: true)
-      testSubPath("i*.s1", "i*.s1",               expect: false)
-      testSubPath("ct.s1.0.i3.x", "ct.s1.0.i3.x", expect: true)
-      testSubPath("c0.s1.0.i3", "c0.s1.0.i3.x",   expect: true)
-      testSubPath("s1.0.i3.x", "s1.0.i3",         expect: false)
-      testSubPath("v**.s1", "v**.s1",             expect: false)
-      testSubPath("i*", "i*",                     expect: false)
-    }
-
-    func testSubPath(_ lhsStr: String, _ rhsStr: String, expect: Bool) {
-      var lhsParser = StringParser(lhsStr)
-      let lhs = try! lhsParser.parseProjectionPathFromSIL()
-      var rhsParser = StringParser(rhsStr)
-      let rhs = try! rhsParser.parseProjectionPathFromSIL()
-      let result = lhs.isSubPath(of: rhs)
-      assert(result == expect)
-    }
-
     func predicates() {
       testPredicate("v**",           \.hasClassProjection, expect: false)
       testPredicate("v**.c0.s1.v**", \.hasClassProjection, expect: true)
