Merge pull request #79605 from atrick/fix-borrowedfrom

Fix BorrowedFromInst operand ownership.

Author: atrick

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DestroyHoisting.swift

@@ -91,7 +91,9 @@ private func optimize(value: Value, _ context: FunctionPassContext) {
   var hoistableDestroys = selectHoistableDestroys(of: value, context)
   defer { hoistableDestroys.deinitialize() }
 
-  var minimalLiverange = InstructionRange(withLiverangeOf: value, ignoring: hoistableDestroys, context)
+  guard var minimalLiverange = InstructionRange(withLiverangeOf: value, ignoring: hoistableDestroys, context) else {
+    return
+  }
   defer { minimalLiverange.deinitialize() }
 
   hoistDestroys(of: value, toEndOf: minimalLiverange, restrictingTo: &hoistableDestroys, context)
@@ -177,10 +179,10 @@ private func removeDestroys(
 
 private extension InstructionRange {
 
-  init(withLiverangeOf initialDef: Value, ignoring ignoreDestroys: InstructionSet, _ context: FunctionPassContext)
+  init?(withLiverangeOf initialDef: Value, ignoring ignoreDestroys: InstructionSet, _ context: FunctionPassContext)
   {
     var liverange = InstructionRange(for: initialDef, context)
-    var visitor = InteriorUseWalker(definingValue: initialDef, ignoreEscape: true, visitInnerUses: false, context) {
+    var visitor = InteriorUseWalker(definingValue: initialDef, ignoreEscape: false, visitInnerUses: true, context) {
       if !ignoreDestroys.contains($0.instruction) {
         liverange.insert($0.instruction)
       }
@@ -197,7 +199,10 @@ private extension InstructionRange {
       return .continueWalk
     }
 
-    _ = visitor.visitUses()
+    guard visitor.visitUses() == .continueWalk else {
+      liverange.deinitialize()
+      return nil
+    }
     self = liverange
   }
 

SwiftCompilerSources/Sources/Optimizer/Utilities/AddressUtils.swift

@@ -554,8 +554,8 @@ extension AddressOwnershipLiveRange {
   ///
   /// For address values, use AccessBase.computeOwnershipRange.
   ///
-  /// FIXME: This should use computeLinearLiveness rather than computeKnownLiveness as soon as lifetime completion
-  /// runs immediately after SILGen.
+  /// FIXME: This should use computeLinearLiveness rather than computeKnownLiveness as soon as complete OSSA lifetimes
+  /// are verified.
   private static func computeValueLiveRange(of value: Value, _ context: FunctionPassContext)
     -> AddressOwnershipLiveRange? {
     switch value.ownership {

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

@@ -134,27 +134,26 @@ import SIL
 
 /// A scoped instruction that borrows one or more operands.
 ///
-/// If this instruction produces a borrowed value, then
-/// BeginBorrowValue(resultOf: self) != nil.
+/// If this instruction produces a borrowed value, then BeginBorrowValue(resultOf: self) != nil.
 ///
-/// This does not include instructions like `apply` and `try_apply` that
-/// instantaneously borrow a value from the caller.
+/// This does not include instructions like `apply` and `try_apply` that instantaneously borrow a value from the caller.
 ///
-/// This does not include `load_borrow` because it borrows a memory
-/// location, not the value of its operand.
+/// This does not include `load_borrow` because it borrows a memory location, not the value of its operand.
 ///
 /// Note: This must handle all instructions with a .borrow operand ownership.
 ///
-/// Note: mark_dependence is a BorrowingInstruction because it creates
-/// a borrow scope for its base operand. Its result, however, is not a
-/// BeginBorrowValue. It is instead a ForwardingInstruction relative
-/// to its value operand.
+/// Note: borrowed_from is a BorrowingInstruction because it creates a borrow scope for its enclosing operands. Its
+/// result, however, is only a BeginBorrowValue (.reborrow) if it forwards a reborrow phi. Otherwise, it simply forwards
+/// a guaranteed value and does not introduce a separate borrow scope.
 ///
-/// TODO: replace BorrowIntroducingInstruction
+/// Note: mark_dependence [nonescaping] is a BorrowingInstruction because it creates a borrow scope for its base
+/// operand. Its result, however, is not a BeginBorrowValue. Instead it is a ForwardingInstruction relative to its value
+/// operand.
 ///
-/// TODO: Add non-escaping MarkDependence.
+/// TODO: replace BorrowIntroducingInstruction with this.
 enum BorrowingInstruction : CustomStringConvertible, Hashable {
   case beginBorrow(BeginBorrowInst)
+  case borrowedFrom(BorrowedFromInst)
   case storeBorrow(StoreBorrowInst)
   case beginApply(BeginApplyInst)
   case partialApply(PartialApplyInst)
@@ -165,13 +164,18 @@ enum BorrowingInstruction : CustomStringConvertible, Hashable {
     switch inst {
     case let bbi as BeginBorrowInst:
       self = .beginBorrow(bbi)
+    case let bfi as BorrowedFromInst:
+      self = .borrowedFrom(bfi)
     case let sbi as StoreBorrowInst:
       self = .storeBorrow(sbi)
     case let bai as BeginApplyInst:
       self = .beginApply(bai)
     case let pai as PartialApplyInst where !pai.mayEscape:
       self = .partialApply(pai)
     case let mdi as MarkDependenceInst:
+      guard mdi.isNonEscaping else {
+        return nil
+      }
       self = .markDependence(mdi)
     case let bi as BuiltinInst
            where bi.id == .StartAsyncLetWithLocalBuffer:
@@ -185,6 +189,8 @@ enum BorrowingInstruction : CustomStringConvertible, Hashable {
     switch self {
     case .beginBorrow(let bbi):
       return bbi
+    case .borrowedFrom(let bfi):
+      return bfi
     case .storeBorrow(let sbi):
       return sbi
     case .beginApply(let bai):
@@ -198,65 +204,88 @@ enum BorrowingInstruction : CustomStringConvertible, Hashable {
     }
   }
 
-  /// Visit the operands that end the local borrow scope.
-  ///
-  /// Note: When this instruction's result is BeginBorrowValue the
-  /// scopeEndingOperand may include reborrows. To find all uses that
-  /// contribute to liveness, the caller needs to determine whether an
-  /// incoming value dominates or is consumed by an outer adjacent
-  /// phi. See InteriorLiveness.
-  ///
-  /// FIXME: To generate conservatively correct liveness, this should return
-  /// .abortWalk if this is a mark_dependence and the scope-ending use is not
-  /// the last in the function (e.g. a store rather than a destroy or return).
-  /// The client needs to use LifetimeDependenceDefUseWalker to do better.
-  ///
-  /// TODO: to handle reborrow-extended uses, migrate ExtendedLiveness
-  /// to SwiftCompilerSources.
-  ///
-  /// TODO: Handle .partialApply and .markDependence forwarded uses
-  /// that are phi operands. Currently, partial_apply [on_stack]
-  /// and mark_dependence [nonescaping] cannot be cloned, so walking
-  /// through the phi safely returns dominated scope-ending operands.
-  /// Instead, this could report the phi as a scope-ending use, and
-  /// the client could decide whether to walk through them or to
-  /// construct reborrow-extended liveness.
-  ///
-  /// TODO: For instructions that are not a BeginBorrowValue, verify
-  /// that scope ending instructions exist on all paths. These
-  /// instructions should be complete after SILGen and never cloned to
-  /// produce phis.
-  func visitScopeEndingOperands(_ context: Context,
-                                visitor: @escaping (Operand) -> WalkResult)
-  -> WalkResult {
+  var innerValue: Value? {
+    if let dependent = dependentValue {
+      return dependent
+    }
+    return scopedValue
+  }
+
+  var dependentValue: Value? {
+    switch self {
+    case .borrowedFrom(let bfi):
+      let phi = bfi.borrowedPhi
+      if phi.isReborrow {
+        return nil
+      }
+      return phi.value
+    case .markDependence(let mdi):
+      if mdi.hasScopedLifetime {
+        return nil
+      }
+      return mdi
+    case .beginBorrow, .storeBorrow, .beginApply, .partialApply, .startAsyncLet:
+      return nil
+    }
+  }
+
+  /// If this is valid, then visitScopeEndingOperands succeeds.
+  var scopedValue: Value? {
     switch self {
     case .beginBorrow, .storeBorrow:
-      let svi = instruction as! SingleValueInstruction
-      return svi.uses.filterUsers(ofType: EndBorrowInst.self).walk {
-        visitor($0)
+      return instruction as! SingleValueInstruction
+    case let .borrowedFrom(bfi):
+      let phi = bfi.borrowedPhi
+      guard phi.isReborrow else {
+        return nil
       }
+      return phi.value
     case .beginApply(let bai):
-      return bai.token.uses.walk { return visitor($0) }
-    case .partialApply, .markDependence:
-      let svi = instruction as! SingleValueInstruction
-      assert(svi.ownership == .owned)
-      return visitForwardedUses(introducer: svi, context) {
-        switch $0 {
-        case let .operand(operand):
-          if operand.endsLifetime {
-            return visitor(operand)
-          }
-          return .continueWalk
-        case let .deadValue(_, operand):
-          if let operand = operand {
-            assert(!operand.endsLifetime,
-                   "a dead forwarding instruction cannot end a lifetime")
-          }
-          return .continueWalk
-        }
+      return bai.token
+    case .partialApply(let pai):
+      // We currently assume that closure lifetimes are always complete (destroyed on all paths).
+      return pai
+    case .markDependence(let mdi):
+      guard mdi.hasScopedLifetime else {
+        return nil
       }
+      return mdi
     case .startAsyncLet(let builtin):
-      return builtin.uses.walk {
+      return builtin
+    }
+  }
+
+  /// Visit the operands that end the local borrow scope.
+  ///
+  /// Returns .abortWalk if the borrow scope cannot be determined from lifetime-ending uses. For example:
+  /// - borrowed_from where 'borrowedPhi.isReborrow == false'
+  /// - non-owned mark_dependence [nonescaping]
+  /// - owned mark_dependence [nonescaping] with a ~Escapable result (LifetimeDependenceDefUseWalker is needed).
+  ///
+  /// Note: .partialApply and .markDependence cannot currently be forwarded to phis because partial_apply [on_stack] and
+  /// mark_dependence [nonescaping] cannot be cloned. Walking through the phi therefore safely returns dominated
+  /// scope-ending operands. Handling phis here requires the equivalent of borrowed_from for owned values.
+  ///
+  /// TODO: For instructions that are not a BeginBorrowValue, verify that scope ending instructions exist on all
+  /// paths. These instructions should be complete after SILGen and never cloned to produce phis.
+  func visitScopeEndingOperands(_ context: Context, visitor: @escaping (Operand) -> WalkResult) -> WalkResult {
+    guard let val = scopedValue else {
+      return .abortWalk
+    }
+    switch self {
+    case .beginBorrow, .storeBorrow:
+      return visitEndBorrows(value: val, context, visitor)
+    case .borrowedFrom:
+      return visitEndBorrows(value: val, context, visitor)
+    case .beginApply:
+      return val.uses.walk { return visitor($0) }
+    case .partialApply:
+      // We currently assume that closure lifetimes are always complete (destroyed on all paths).
+      return visitOwnedDependent(value: val, context, visitor)
+    case .markDependence:
+      return visitOwnedDependent(value: val, context, visitor)
+    case .startAsyncLet:
+      return val.uses.walk {
         if let builtinUser = $0.instruction as? BuiltinInst,
           builtinUser.id == .EndAsyncLetLifetime {
           return visitor($0)
@@ -265,6 +294,34 @@ enum BorrowingInstruction : CustomStringConvertible, Hashable {
       }
     }
   }
+}
+
+extension BorrowingInstruction {
+  private func visitEndBorrows(value: Value, _ context: Context, _ visitor: @escaping (Operand) -> WalkResult)
+    -> WalkResult {
+    return value.lookThroughBorrowedFromUser.uses.filterUsers(ofType: EndBorrowInst.self).walk {
+      visitor($0)
+    }
+  }
+
+  private func visitOwnedDependent(value: Value, _ context: Context, _ visitor: @escaping (Operand) -> WalkResult)
+    -> WalkResult {
+    return visitForwardedUses(introducer: value, context) {
+      switch $0 {
+      case let .operand(operand):
+        if operand.endsLifetime {
+          return visitor(operand)
+        }
+        return .continueWalk
+      case let .deadValue(_, operand):
+        if let operand = operand {
+          assert(!operand.endsLifetime,
+                 "a dead forwarding instruction cannot end a lifetime")
+        }
+        return .continueWalk
+      }
+    }
+  }
 
   var description: String { instruction.description }
 }
@@ -341,9 +398,12 @@ enum BeginBorrowValue {
   init?(resultOf borrowInstruction: BorrowingInstruction) {
     switch borrowInstruction {
     case let .beginBorrow(beginBorrow):
-      self = BeginBorrowValue(beginBorrow)!
+      self.init(beginBorrow)
+    case let .borrowedFrom(borrowedFrom):
+      // only returns non-nil if borrowedPhi is a reborrow
+      self.init(borrowedFrom.borrowedPhi.value)
     case let .beginApply(beginApply):
-      self = BeginBorrowValue(beginApply.token)!
+      self.init(beginApply.token)
     case .storeBorrow, .partialApply, .markDependence, .startAsyncLet:
       return nil
     }

SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift

@@ -771,12 +771,14 @@ extension LifetimeDependenceDefUseWalker {
     switch borrowInst {
     case let .beginBorrow(bbi):
       return walkDownUses(of: bbi, using: operand)
+    case let .borrowedFrom(bfi):
+      return walkDownUses(of: bfi, using: operand)
     case let .storeBorrow(sbi):
       return walkDownAddressUses(of: sbi)
     case .beginApply:
       // Skip the borrow scope; the type system enforces non-escapable
       // arguments.
-      return visitInnerBorrowUses(of: borrowInst)
+      return visitInnerBorrowUses(of: borrowInst, operand: operand)
     case .partialApply, .markDependence:
       fatalError("OwnershipUseVisitor should bypass partial_apply [on_stack] "
                  + "and mark_dependence [nonescaping]")