Fix LifetimeDependenceScopeFixup: read access can depend on 'inout'

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceScopeFixup.swift

@@ -453,30 +453,30 @@ extension ScopeExtension {
   /// logically combine them into a single access for the purpose of diagnostic lifetime dependence.
   var dependsOnArgs: SingleInlineArray<FunctionArgument> {
     let noCallerScope = SingleInlineArray<FunctionArgument>()
+    // Check that the dependent value is returned by this function.
     if !dependsOnCaller! {
       return noCallerScope
     }
-    // All owners must be arguments to depend on the caller's scope.
+    // Check that all nested scopes that it depends on can be covered by exclusive access in the caller.
+    for extScope in scopes {
+      switch extScope.scope {
+      case .access:
+        break
+      default:
+        return noCallerScope
+      }
+    }
+    // All owners must be arguments with exclusive access to depend on the caller's scope (inout_aliasable arguments do
+    // not have exclusivity).
     var compatibleArgs = SingleInlineArray<FunctionArgument>()
     for owner in owners {
       guard let arg = owner as? FunctionArgument else {
         return noCallerScope
       }
-      compatibleArgs.push(arg)
-    }
-    // The only local scopes that are compatible with the caller's scope are access scopes that either read an
-    // immutable argument or modify a mutable argument.
-    for extScope in scopes {
-      switch extScope.scope {
-      case let .access(beginAccess):
-        for arg in compatibleArgs {
-          if !beginAccess.accessKind.isCompatible(with: arg.convention) {
-            return noCallerScope
-          }
-        }
-      default:
+      guard arg.convention.isIndirectIn || arg.convention.isInout else {
         return noCallerScope
       }
+      compatibleArgs.push(arg)
     }
     return compatibleArgs
   }
@@ -762,13 +762,20 @@ extension ExtendableScope {
     defer { unusedEnds.deinitialize() }
     for end in range.ends {
       let location = end.location.autoGenerated
-      if end is ReturnInst {
+      switch end {
+      case is BranchInst:
+        assert(end.parentBlock.singleSuccessor!.terminator is ReturnInst,
+               "a phi only ends a use range if it is a returned value")
+        fallthrough
+      case is ReturnInst:
         // End this inner scope just before the return. The mark_dependence base operand will be redirected to a
         // function argument.
         let builder = Builder(before: end, location: location, context)
         // Insert newEnd so that this scope will be nested in any outer scopes.
         range.insert(createEndInstruction(builder, context))
         continue
+      default:
+        break
       }
       if unusedEnds.contains(end) {
         unusedEnds.erase(end)

SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift

@@ -587,7 +587,7 @@ extension LifetimeDependenceDefUseWalker {
   // Override ForwardingDefUseWalker.
   mutating func walkDown(operand: Operand) -> WalkResult {
     // Initially delegate all uses to OwnershipUseVisitor.
-    // walkDownDefault will be called for uses that forward ownership.
+    // forwardingUse() will be called for uses that forward ownership, which then delegates to walkDownDefault().
     return classify(operand: operand)
   }
 
@@ -665,6 +665,11 @@ extension LifetimeDependenceDefUseWalker {
 
   mutating func forwardingUse(of operand: Operand, isInnerLifetime: Bool)
     -> WalkResult {
+    // Lifetime dependence is only interested in dominated uses. Treat a returned phi like a dominated use by stopping
+    // at the phi operand rather than following the forwarded value to the ReturnInst.
+    if let phi = Phi(using: operand), phi.isReturnValue {
+      return returnedDependence(result: operand)
+    }
     // Delegate ownership forwarding operations to the ForwardingDefUseWalker.
     return walkDownDefault(forwarding: operand)
   }

SwiftCompilerSources/Sources/SIL/Argument.swift

@@ -192,6 +192,22 @@ public struct Phi {
   }
 }
 
+extension Phi {
+  /// Return true of this phi is directly returned with no side effects between the phi and the return.
+  public var isReturnValue: Bool {
+    if let singleUse = value.uses.singleUse, let ret = singleUse.instruction as? ReturnInst,
+       ret.parentBlock == successor {
+      for inst in successor.instructions {
+        if inst.mayHaveSideEffects {
+          return false
+        }
+      }
+      return true
+    }
+    return false
+  }
+}
+
 extension Operand {
   public var forwardingBorrowedFromUser: BorrowedFromInst? {
     if let bfi = instruction as? BorrowedFromInst, index == 0 {
@@ -591,19 +607,6 @@ public enum ArgumentConvention : CustomStringConvertible {
   }
 }
 
-extension BeginAccessInst.AccessKind {
-  public func isCompatible(with convention: ArgumentConvention) -> Bool {
-    switch self {
-    case .`init`, .deinit:
-      return false
-    case .read:
-      return convention.isIndirectIn
-    case .modify:
-      return convention.isInout
-    }
-  }
-}
-
 // Bridging utilities
 
 extension BridgedArgument {

test/SILOptimizer/lifetime_dependence/scope_fixup.sil

@@ -64,7 +64,9 @@ sil @getNEPointerToA : $@convention(thin) (@guaranteed NE) -> UnsafePointer<A>
 sil @useA : $@convention(thin) (A) -> ()
 
 sil @getPtr : $@convention(thin) () -> @out UnsafeRawPointer
-sil @getSpan : $@convention(thin) (@in_guaranteed AnyObject) -> @lifetime(borrow 0) @out NE
+
+sil @getOwnedNE : $@convention(thin) (@guaranteed Holder) -> @lifetime(borrow 0) @owned NE
+sil @getOwnedNEFromInout : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned NE
 
 sil @useNE : $@convention(thin) (@guaranteed NE) -> ()
 
@@ -439,3 +441,66 @@ bb0(%0 : @owned $B):
   %24 = tuple ()
   return %24
 }
+
+// =============================================================================
+// Return value extension
+// =============================================================================
+
+// Sink the end access to the "return" phi and rewrite mark_dependence on the argument.
+//
+// CHECK-LABEL: sil hidden [ossa] @testReturnPhi : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned Optional<NE> {
+// CHECK: bb0(%0 : $*Holder):
+// CHECK: bb1:
+// CHECK:   enum $Optional<NE>, #Optional.none!enumelt
+// CHECK: bb2:
+// CHECK:   [[ACCESS:%.*]] = begin_access [modify] [unknown] %0
+// CHECK:   [[MD:%.*]] = mark_dependence [unresolved] {{%.*}} on %0
+// CHECK:   [[OPT:%.*]] = enum $Optional<NE>, #Optional.some!enumelt, %7
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   br bb3([[OPT]])
+// CHECK: bb3([[RET:%.*]] : @owned $Optional<NE>):
+// CHECK:   return [[RET]]
+// CHECK-LABEL: } // end sil function 'testReturnPhi'
+sil hidden [ossa] @testReturnPhi : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned Optional<NE> {
+bb0(%0 : $*Holder):
+  cond_br undef, bb1, bb2
+
+bb1:
+  %none = enum $Optional<NE>, #Optional.none!enumelt
+  br bb3(%none)
+
+bb2:
+  %access = begin_access [modify] [unknown] %0
+  %f = function_ref @getOwnedNEFromInout : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned NE
+  %ne = apply %f(%access) : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned NE
+  %md = mark_dependence [unresolved] %ne on %access
+  end_access %access
+  %some = enum $Optional<NE>, #Optional.some!enumelt, %md
+  br bb3(%some)
+
+bb3(%ret : @owned $Optional<NE>):
+  return %ret
+}
+
+// Allow a [read] access to depend on a caller's [modify] access.
+//
+// CHECK-LABEL: sil hidden [noinline] [ossa] @testNestedModRead : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned NE {
+// CHECK: bb0(%0 : $*Holder):
+// CHECK:   [[ACCESS:%.*]] = begin_access [read] [unknown] %0
+// CHECK:   [[LD:%.*]] = load [copy] [[ACCESS]]
+// CHECK:   [[MD:%.*]] = mark_dependence [unresolved] %{{.*}} on %0
+// CHECK:   destroy_value [[LD]]
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   return [[MD]]
+// CHECK-LABEL: } // end sil function 'testNestedModRead'
+sil hidden [noinline] [ossa] @testNestedModRead : $@convention(thin) (@inout Holder) -> @lifetime(borrow 0) @owned NE {
+bb0(%0 : $*Holder):
+  %access = begin_access [read] [unknown] %0
+  %holder = load [copy] %access
+  end_access %access
+  %f = function_ref @getOwnedNE : $@convention(thin) (@guaranteed Holder) -> @lifetime(borrow 0) @owned NE
+  %ne = apply %f(%holder) : $@convention(thin) (@guaranteed Holder) -> @lifetime(borrow 0) @owned NE
+  %md = mark_dependence [unresolved] %ne on %access
+  destroy_value %holder
+  return %md
+}

test/SILOptimizer/lifetime_dependence/semantics.swift

@@ -434,9 +434,8 @@ func testGlobal(local: InnerTrivial) -> Span<Int> {
 
 @lifetime(&a)
 func testInoutBorrow(a: inout [Int]) -> Span<Int> {
-  a.span() // expected-error {{lifetime-dependent value escapes its scope}}
-  // expected-note @-1{{it depends on this scoped access to variable 'a'}}
-} // expected-note {{this use causes the lifetime-dependent value to escape}}
+  a.span() // OK
+}
 
 @lifetime(&a)
 func testInoutMutableBorrow(a: inout [Int]) -> MutableSpan<Int> {
@@ -462,12 +461,10 @@ extension Container {
 
   @lifetime(&self)
   mutating func mutableView() -> MutableView {
-    // Reading 'self.owner' creates a local borrow scope. This new MutableView
-    // depends on a the local borrow scope for 'self.owner', so it cannot be
-    // returned.
-    MutableView(owner: self.owner) // expected-error {{lifetime-dependent value escapes its scope}}
-    // expected-note @-1{{it depends on this scoped access to variable 'self'}}
-  } // expected-note    {{this use causes the lifetime-dependent value to escape}}
+    // Reading 'self.owner' creates a local borrow scope. The caller's exclusive access on 'self' is sufficient for the
+    // result.
+    MutableView(owner: self.owner) // OK
+  }
 
   @lifetime(&self)
   mutating func mutableViewModifiesOwner() -> MutableView {

test/SILOptimizer/lifetime_dependence/verify_diagnostics.swift

@@ -0,0 +1,37 @@
+// RUN: %target-swift-frontend %s -emit-sil \
+// RUN:   -o /dev/null \
+// RUN:   -verify \
+// RUN:   -sil-verify-all \
+// RUN:   -module-name test \
+// RUN:   -define-availability "Span 0.1:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, visionOS 9999" \
+// RUN:   -enable-experimental-feature LifetimeDependence
+
+// REQUIRES: swift_in_compiler
+// REQUIRES: swift_feature_LifetimeDependence
+
+// Test diagnostic output for interesting corner cases. Similar to semantics.swift, but this tests corner cases in the
+// implementation as opposed to basic language rules.
+
+// Test that conditionally returning an Optional succeeds.
+//
+// See scope_fixup.sil: testReturnPhi.
+@available(Span 0.1, *)
+extension Array {
+  @lifetime(&self)
+  mutating func getOptionalMutableSpan() -> MutableSpan<Element>? {
+    if count == 0 {
+      return nil
+    }
+    return mutableSpan
+  }
+}
+
+// Test that returning an immutable Span from an inout Array.
+//
+// See scope_fixup.sil: testNestedModRead.
+@available(Span 0.1, *)
+@inline(never)
+@lifetime(&array)
+func getImmutableSpan(_ array: inout [Int]) -> Span<Int> {
+ return array.span
+}