Merge pull request #85533 from eeckstein/fix-access-simplification

SILOptimizer: don't remove empty conflicting access scopes

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/CMakeLists.txt

@@ -17,6 +17,7 @@ swift_compiler_sources(Optimizer
   ComputeEscapeEffects.swift
   ComputeSideEffects.swift
   CopyToBorrowOptimization.swift
+  DeadAccessScopeElimination.swift
   DeadStoreElimination.swift
   DeinitDevirtualizer.swift
   DestroyHoisting.swift

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DeadAccessScopeElimination.swift

@@ -0,0 +1,247 @@
+//===--- DeadAccessScopeElimination.swift ----------------------------------==//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2025 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SIL
+
+/// Eliminates dead access scopes if they are not conflicting with other scopes.
+///
+/// Removes:
+/// ```
+///   %2 = begin_access [modify] [dynamic] %1
+///   ...                                       // no uses of %2
+///   end_access %2
+/// ```
+///
+/// However, dead _conflicting_ access scopes are not removed.
+/// If a conflicting scope becomes dead because an optimization e.g. removed a load, it is still
+/// important to get an access violation at runtime.
+/// Even a propagated value of a redundant load from a conflicting scope is undefined.
+///
+/// ```
+///   %2 = begin_access [modify] [dynamic] %1
+///   store %x to %2
+///   %3 = begin_access [read] [dynamic] %1    // conflicting with %2!
+///   %y = load %3
+///   end_access %3
+///   end_access %2
+///   use(%y)
+/// ```
+/// After redundant-load-elimination:
+/// ```
+///   %2 = begin_access [modify] [dynamic] %1
+///   store %x to %2
+///   %3 = begin_access [read] [dynamic] %1    // now dead, but still conflicting with %2
+///   end_access %3
+///   end_access %2
+///   use(%x)                                  // propagated from the store, but undefined here!
+/// ```
+/// In this case the scope `%3` is not removed because it's important to get an access violation
+/// error at runtime before the undefined value `%x` is used.
+///
+/// This pass considers potential conflicting access scopes in called functions.
+/// But it does not consider potential conflicting access in callers (because it can't!).
+/// However, optimizations, like redundant-load-elimination, can only do such transformations if
+/// the outer access scope is within the function, e.g.
+///
+/// ```
+/// bb0(%0 : $*T):     // an inout from a conflicting scope in the caller
+///   store %x to %0
+///   %3 = begin_access [read] [dynamic] %1
+///   %y = load %3     // cannot be propagated because it cannot be proved that %1 is the same address as %0
+///   end_access %3
+/// ```
+///
+/// All those checks are only done for dynamic access scopes, because they matter for runtime
+/// exclusivity checking. Dead static scopes are removed unconditionally.
+///
+let deadAccessScopeElimination = FunctionPass(name: "dead-access-scope-elimination") {
+  (function: Function, context: FunctionPassContext) in
+
+  // Add all dead scopes here and then remove the ones which turn out to be conflicting.
+  var removableScopes = SpecificIterableInstructionSet<BeginAccessInst>(context)
+  defer { removableScopes.deinitialize() }
+
+  var scopeTree = ScopeTree(context)
+
+  // The payload is the recent access instruction at the block begin, e.g.
+  // ```
+  //   %1 = begin_acces %0
+  //   br bb2
+  // bb2:                  // recent access instruction at begin of bb2 is: `%1 = begin_acces %0`
+  // ```
+  // It's nil if the block is not within an access scope.
+  //
+  var blockWorklist = BasicBlockWorklistWithPayload<Instruction?>(context)
+  defer { blockWorklist.deinitialize() }
+
+  blockWorklist.pushIfNotVisited(function.entryBlock, with: nil)
+
+  // Walk through the control flow in depth-first order. Note that we don't need to do any kind
+  // of state merging at merge-points, because access scopes must be consistent on all paths.
+  while let (block, recentAccessInstAtBlockBegin) = blockWorklist.pop() {
+
+    // The last seen `begin_access` (or `end_access` in case of not perfectly nested scopes; see ScopeTree.backlinks)
+    var recentAccessInst = recentAccessInstAtBlockBegin
+
+    for inst in block.instructions {
+      process(instruction: inst, updating: &recentAccessInst, &scopeTree, &removableScopes)
+    }
+
+    blockWorklist.pushIfNotVisited(contentsOf: block.successors, with: recentAccessInst)
+  }
+
+  for deadBeginAccess in removableScopes {
+    context.erase(instructionIncludingAllUsers: deadBeginAccess)
+  }
+}
+
+private func process(instruction: Instruction,
+                     updating recentAccessInst: inout Instruction?,
+                     _ scopeTree: inout ScopeTree,
+                     _ removableScopes: inout SpecificIterableInstructionSet<BeginAccessInst>)
+{
+  switch instruction {
+
+  case let beginAccess as BeginAccessInst:
+    if beginAccess.isDead {
+      // Might be removed again later if it turns out to be in a conflicting scope.
+      removableScopes.insert(beginAccess)
+    }
+    if beginAccess.enforcement != .dynamic {
+      // We unconditionally remove dead _static_ scopes, because they don't have any impact at runtime.
+      // Usually static scopes are already removed in the optimization pipeline. However optimizations
+      // might turn dynamic into static scopes. So let's handle them.
+      break
+    }
+    scopeTree.visitEnclosingScopes(of: recentAccessInst) { enclosingBeginAccess in
+      if beginAccess.accessKind.conflicts(with: enclosingBeginAccess.accessKind),
+         // Avoid computing alias info if both scopes are not removable anyway.
+         removableScopes.contains(beginAccess) || removableScopes.contains(enclosingBeginAccess),
+         scopeTree.context.aliasAnalysis.mayAlias(beginAccess.address, enclosingBeginAccess.address)
+      {
+        // Conflicting enclosing scopes are not removable.
+        removableScopes.erase(enclosingBeginAccess)
+        // ... as well as the inner scope (which conflicts with the enclosing scope).
+        removableScopes.erase(beginAccess)
+      }
+    }
+    scopeTree.update(recent: &recentAccessInst, with: beginAccess)
+
+  case let endAccess as EndAccessInst where endAccess.beginAccess.enforcement == .dynamic:
+    scopeTree.update(recent: &recentAccessInst, with: endAccess)
+
+  default:
+    if instruction.mayCallFunction {
+      // Check for potential conflicting scopes in called functions.
+      scopeTree.visitEnclosingScopes(of: recentAccessInst) { enclosingBeginAccess in
+        if removableScopes.contains(enclosingBeginAccess),
+           instruction.mayHaveAccessScopeWhichConflicts(with: enclosingBeginAccess, scopeTree.context)
+        {
+          removableScopes.erase(enclosingBeginAccess)
+        }
+      }
+    }
+  }
+}
+
+/// Represents the tree of access scopes in a function.
+/// Note that if the scopes are not nested perfectly, it's strictly speaking not a tree.
+private struct ScopeTree {
+
+  // Links `begin_access` and `end_access` instructions in backward control flow direction.
+  // This is used to visit all enclosing scopes of a `begin_access`.
+  // As an optimization, `end_access`es are ignored for scopes which are perfectly nested - which is
+  // by far the most common case. In this case the backlinks simply are the parent links in the scope tree.
+  //
+  // Example of not perfectly nested scopes:
+  // ```
+  //   %1 = begin_access    <------------------+
+  //   ...                                     |
+  //   %2 = begin_access    <--------------+  -+
+  //   ...                                 |
+  //   end_access %1        <---------+   -+
+  //   ...                            |
+  //   %3 = begin_access    <-----+  -+
+  //   ...                        |
+  //   end_access %2        <-+  -+
+  //   ...                    |
+  //   end_access %3         -+
+  // ```
+  //
+  // Perfectly nested scopes:
+  // ```
+  //   %1 = begin_access    <-+   <-+
+  //   ...                    |     |
+  //   %2 = begin_access     -+     |
+  //   end_access %2                |    <- ignored
+  //   ...                          |
+  //   %3 = begin_access     -------+
+  //   end_access %3                     <- ignored
+  //   ...
+  //   end_access %1                     <- ignored
+  // ```
+  private var backlinks = Dictionary<Instruction, Instruction>()
+
+  let context: FunctionPassContext
+
+  init(_ context: FunctionPassContext) { self.context = context }
+
+  mutating func update(recent: inout Instruction?, with beginAccess: BeginAccessInst) {
+    backlinks[beginAccess] = recent
+    recent = beginAccess
+  }
+
+  mutating func update(recent: inout Instruction?, with endAccess: EndAccessInst) {
+    if endAccess.beginAccess == recent {
+      // The scope is perfectly nested. Ignore it and directly backlink to the parent of the `begin_access`
+      recent = backlinks[endAccess.beginAccess]
+    } else {
+      backlinks[endAccess] = recent
+      recent = endAccess
+    }
+  }
+
+  func visitEnclosingScopes(of accessInstruction: Instruction?, closure: (BeginAccessInst) -> ()) {
+    // Ignore scopes which are already closed
+    var ignore = SpecificInstructionSet<BeginAccessInst>(context)
+    defer { ignore.deinitialize() }
+
+    var enclosingScope = accessInstruction
+
+    while let parent = enclosingScope {
+      switch parent {
+      case let parentBeginAccess as BeginAccessInst where !ignore.contains(parentBeginAccess):
+        closure(parentBeginAccess)
+      case let parentEndAccess as EndAccessInst:
+        // Seeing an `end_access` in the backlink chain means that this scope is already closed.
+        ignore.insert(parentEndAccess.beginAccess)
+      default:
+        break
+      }
+      enclosingScope = backlinks[parent]
+    }
+  }
+}
+
+private extension Instruction {
+  func mayHaveAccessScopeWhichConflicts(with beginAccess: BeginAccessInst, _ context: FunctionPassContext) -> Bool {
+    if beginAccess.accessKind == .read {
+      return mayWrite(toAddress: beginAccess.address, context.aliasAnalysis)
+    } else {
+      return mayReadOrWrite(address: beginAccess.address, context.aliasAnalysis)
+    }
+  }
+}
+
+private extension BeginAccessInst {
+  var isDead: Bool { users.allSatisfy({ $0.isIncidentalUse }) }
+}

SwiftCompilerSources/Sources/Optimizer/PassManager/PassRegistration.swift

@@ -94,6 +94,7 @@ private func registerSwiftPasses() {
   registerPass(cleanupDebugStepsPass, { cleanupDebugStepsPass.run($0) })
   registerPass(namedReturnValueOptimization, { namedReturnValueOptimization.run($0) })
   registerPass(stripObjectHeadersPass, { stripObjectHeadersPass.run($0) })
+  registerPass(deadAccessScopeElimination, { deadAccessScopeElimination.run($0) })
   registerPass(deadStoreElimination, { deadStoreElimination.run($0) })
   registerPass(redundantLoadElimination, { redundantLoadElimination.run($0) })
   registerPass(mandatoryRedundantLoadElimination, { mandatoryRedundantLoadElimination.run($0) })

SwiftCompilerSources/Sources/SIL/DataStructures/Set.swift

@@ -287,3 +287,66 @@ extension IntrusiveSet {
     insert(contentsOf: source)
   }
 }
+
+/// A set which supports iterating over its elements.
+/// The iteration order is deterministic. All set operations are O(1).
+public struct IterableSet<Set: IntrusiveSet> : CollectionLikeSequence {
+  public typealias Element = Set.Element
+
+  private var set: Set
+
+  // Erased elements do not get removed from `list`, because this would be O(n).
+  private var list: Stack<Element>
+
+  // Elements which are in `list. This is different from `set` if elements get erased.
+  private var inList: Set
+
+  public init(_ context: some Context) {
+    self.set = Set(context)
+    self.list = Stack(context)
+    self.inList = Set(context)
+  }
+
+  public mutating func deinitialize() {
+    inList.deinitialize()
+    list.deinitialize()
+    set.deinitialize()
+  }
+
+  public func contains(_ element: Element) -> Bool {
+    set.contains(element)
+  }
+
+  /// Returns true if `element` was not contained in the set before inserting.
+  @discardableResult
+  public mutating func insert(_ element: Element) -> Bool {
+    if inList.insert(element) {
+      list.append(element)
+    }
+    return set.insert(element)
+  }
+
+  public mutating func erase(_ element: Element) {
+    set.erase(element)
+  }
+
+  public func makeIterator() -> Iterator { Iterator(base: list.makeIterator(), set: set) }
+
+  public struct Iterator: IteratorProtocol {
+    var base: Stack<Element>.Iterator
+    let set: Set
+
+    public mutating func next() -> Element? {
+      while let n = base.next() {
+        if set.contains(n) {
+          return n
+        }
+      }
+      return nil
+    }
+  }
+}
+
+public typealias IterableInstructionSet = IterableSet<InstructionSet>
+public typealias SpecificIterableInstructionSet<InstType: Instruction> = IterableSet<SpecificInstructionSet<InstType>>
+public typealias IterableBasicBlockSet = IterableSet<BasicBlockSet>

SwiftCompilerSources/Sources/SIL/DataStructures/Worklist.swift

@@ -70,7 +70,54 @@ public struct Worklist<Set: IntrusiveSet> : CustomStringConvertible, NoReflectio
   }
 }
 
+/// Like `Worklist` but can store an additional arbitrary payload per element.
+public struct WorklistWithPayload<Set: IntrusiveSet, Payload> : CustomStringConvertible, NoReflectionChildren {
+  public typealias Element = Set.Element
+  private var worklist: Stack<(Element, Payload)>
+  private var pushedElements: Set
+
+  public init(_ context: some Context) {
+    self.worklist = Stack(context)
+    self.pushedElements = Set(context)
+  }
+
+  public mutating func pop() -> (Element, Payload)? { return worklist.pop() }
+
+  public mutating func pushIfNotVisited(_ element: Element, with payload: Payload) {
+    if pushedElements.insert(element) {
+      worklist.append((element, payload))
+    }
+  }
+
+  public mutating func pushIfNotVisited<S: Sequence>(contentsOf other: S, with payload: Payload)
+                  where S.Element == Element
+  {
+    for element in other {
+      pushIfNotVisited(element, with: payload)
+    }
+  }
+
+  /// Returns true if \p element was pushed to the worklist, regardless if it's already popped or not.
+  public func hasBeenPushed(_ element: Element) -> Bool { pushedElements.contains(element) }
+
+  public var isEmpty: Bool { worklist.isEmpty }
+
+  public var description: String {
+    """
+    worklist: \(worklist)
+    pushed:   \(pushedElements)
+    """
+  }
+
+  /// TODO: once we have move-only types, make this a real deinit.
+  public mutating func deinitialize() {
+    pushedElements.deinitialize()
+    worklist.deinitialize()
+  }
+}
+
 public typealias BasicBlockWorklist = Worklist<BasicBlockSet>
+public typealias BasicBlockWorklistWithPayload<Payload> = WorklistWithPayload<BasicBlockSet, Payload>
 public typealias InstructionWorklist = Worklist<InstructionSet>
 public typealias SpecificInstructionWorklist<InstType: Instruction> = Worklist<SpecificInstructionSet<InstType>>
 public typealias ValueWorklist = Worklist<ValueSet>

SwiftCompilerSources/Sources/SIL/Instruction.swift

@@ -143,6 +143,11 @@ public class Instruction : CustomStringConvertible, Hashable {
     return bridged.mayAccessPointer()
   }
 
+  /// True if arbitrary functions may be called by this instruction.
+  /// This can be either directly, e.g. by an `apply` instruction, or indirectly by destroying a value which
+  /// might have a deinitializer which can call functions.
+  public var mayCallFunction: Bool { false }
+
   public final var mayLoadWeakOrUnowned: Bool {
     return bridged.mayLoadWeakOrUnowned()
   }
@@ -331,6 +336,8 @@ final public class StoreInst : Instruction, StoringInstruction {
   public var storeOwnership: StoreOwnership {
     StoreOwnership(rawValue: bridged.StoreInst_getStoreOwnership())!
   }
+
+  public override var mayCallFunction: Bool { storeOwnership == .assign }
 }
 
 final public class StoreWeakInst : Instruction, StoringInstruction { }
@@ -345,6 +352,15 @@ final public class AssignInst : Instruction, StoringInstruction {
   public var assignOwnership: AssignOwnership {
     AssignOwnership(rawValue: bridged.AssignInst_getAssignOwnership())!
   }
+
+  public override var mayCallFunction: Bool {
+    switch assignOwnership {
+    case .unknown, .reassign, .reinitialize:
+      true
+    case .initialize:
+      false
+    }
+  }
 }
 
 final public class AssignOrInitInst : Instruction, StoringInstruction {}
@@ -382,6 +398,7 @@ final public class CopyAddrInst : Instruction, SourceDestAddrInstruction {
     bridged.CopyAddrInst_setIsInitializationOfDest(isInitializationOfDestination)
     context.notifyInstructionChanged(self)
   }
+  public override var mayCallFunction: Bool { !isInitializationOfDestination }
 }
 
 final public class ExplicitCopyAddrInst : Instruction, SourceDestAddrInstruction {
@@ -394,6 +411,7 @@ final public class ExplicitCopyAddrInst : Instruction, SourceDestAddrInstruction
   public var isInitializationOfDestination: Bool {
     bridged.ExplicitCopyAddrInst_isInitializationOfDest()
   }
+  public override var mayCallFunction: Bool { !isInitializationOfDestination }
 }
 
 final public class MarkUninitializedInst : SingleValueInstruction, UnaryInstruction {
@@ -639,10 +657,12 @@ final public class RetainValueAddrInst : RefCountingInst {
 }
 
 final public class ReleaseValueAddrInst : RefCountingInst {
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class StrongReleaseInst : RefCountingInst {
   public var instance: Value { operand.value }
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class UnownedReleaseInst : RefCountingInst {
@@ -651,10 +671,12 @@ final public class UnownedReleaseInst : RefCountingInst {
 
 final public class ReleaseValueInst : RefCountingInst {
   public var value: Value { return operand.value }
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class UnmanagedReleaseValueInst : RefCountingInst {
   public var value: Value { return operand.value }
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class AutoreleaseValueInst : RefCountingInst {}
@@ -667,10 +689,14 @@ final public class DestroyValueInst : Instruction, UnaryInstruction {
   /// end the lifetime of its operand.
   /// Such `destroy_value` instructions are lowered to no-ops.
   public var isDeadEnd: Bool { bridged.DestroyValueInst_isDeadEnd() }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class DestroyAddrInst : Instruction, UnaryInstruction {
   public var destroyedAddress: Value { operand.value }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class EndLifetimeInst : Instruction, UnaryInstruction {}
@@ -762,6 +788,15 @@ final public class BuiltinInst : SingleValueInstruction {
   public var arguments: LazyMapSequence<OperandArray, Value> {
     operands.values
   }
+
+  public override var mayCallFunction: Bool {
+    switch id {
+    case .Once, .OnFastPath, .Destroy, .DestroyArray, .DestroyTaskGroup, .DestroyDefaultActor, .Release:
+      true
+    default:
+      false
+    }
+  }
 }
 
 final public class UpcastInst : SingleValueInstruction, UnaryInstruction {
@@ -1371,6 +1406,8 @@ final public class ApplyInst : SingleValueInstruction, FullApplySite {
   public typealias SpecializationInfo = BridgedGenericSpecializationInformation
 
   public var specializationInfo: SpecializationInfo { bridged.ApplyInst_getSpecializationInfo() }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class FunctionExtractIsolationInst : SingleValueInstruction {}
@@ -1648,7 +1685,12 @@ final public class BeginAccessInst : SingleValueInstruction, UnaryInstruction {
     case read = 1
     case modify = 2
     case `deinit` = 3
+
+    public func conflicts(with other: AccessKind) -> Bool {
+      return self != .read || other != .read
+    }
   }
+
   public var accessKind: AccessKind {
     AccessKind(rawValue: bridged.BeginAccessInst_getAccessKind())!
   }
@@ -1730,11 +1772,15 @@ final public class BeginApplyInst : MultipleValueInstruction, FullApplySite {
 
   public var isNonThrowing: Bool { bridged.BeginApplyInst_getNonThrowing() }
   public var isNonAsync: Bool { bridged.BeginApplyInst_getNonAsync() }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class EndApplyInst : SingleValueInstruction, UnaryInstruction {
   public var token: MultipleValueInstructionResult { operand.value as! MultipleValueInstructionResult }
   public var beginApply: BeginApplyInst { token.parentInstruction as! BeginApplyInst }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class AbortApplyInst : Instruction, UnaryInstruction {
@@ -1931,6 +1977,8 @@ final public class TryApplyInst : TermInst, FullApplySite {
 
   public var isNonAsync: Bool { bridged.TryApplyInst_getNonAsync() }
   public var specializationInfo: ApplyInst.SpecializationInfo { bridged.TryApplyInst_getSpecializationInfo() }
+
+  public override var mayCallFunction: Bool { true }
 }
 
 final public class BranchInst : TermInst {