Merge pull request #78768 from eeckstein/fix-borrowed-from-updater

SIL: when updating borrowed-from instructions, remove the old enclosing values first

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/Utilities/PhiUpdater.swift

@@ -50,7 +50,7 @@ func updateBorrowedFrom(for phis: some Sequence<Phi>, _ context: some MutatingCo
       return
     }
     if phi.value.ownership == .guaranteed {
-      createBorrowedFrom(for: phi, context)
+      createEmptyBorrowedFrom(for: phi, context)
     }
   }
 
@@ -113,44 +113,34 @@ private func updateBorrowedFrom(for phi: Phi, _ context: some MutatingContext) -
   defer { computedEVs.deinitialize() }
   gatherEnclosingValuesFromPredecessors(for: phi, in: &computedEVs, context)
 
-  var changed = false
-  for use in phi.value.uses {
-    if let bfi = use.forwardingBorrowedFromUser {
-      changed = addEnclosingValues(computedEVs, to: bfi, context) || changed
-    }
-  }
-  return changed
-}
-
-private func createBorrowedFrom(for phi: Phi, _ context: some MutatingContext) {
-  if !phi.value.uses.contains(where: { $0.forwardingBorrowedFromUser != nil }) {
-    let builder = Builder(atBeginOf: phi.value.parentBlock, context)
-    let bfi = builder.createBorrowedFrom(borrowedValue: phi.value, enclosingValues: [])
-    phi.value.uses.ignoreUsers(ofType: BorrowedFromInst.self).replaceAll(with: bfi, context)
-  }
-}
-
-private func addEnclosingValues(
-  _ newEVs: some Sequence<Value>,
-  to borrowedFrom: BorrowedFromInst,
-  _ context: some MutatingContext) -> Bool
-{
+  let borrowedFrom = phi.borrowedFrom!
   var existingEVs = ValueSet(insertContentsOf: borrowedFrom.enclosingValues, context)
   defer { existingEVs.deinitialize() }
 
-  if newEVs.allSatisfy({ existingEVs.contains($0) }) {
+  if computedEVs.allSatisfy({ existingEVs.contains($0) }) {
     return false
   }
-
   var evs = Array<Value>(borrowedFrom.enclosingValues)
-  evs.append(contentsOf: newEVs.lazy.filter { !existingEVs.contains($0) })
+  evs.append(contentsOf: computedEVs.lazy.filter { !existingEVs.contains($0) })
 
   let builder = Builder(before: borrowedFrom, context)
   let newBfi = builder.createBorrowedFrom(borrowedValue: borrowedFrom.borrowedValue, enclosingValues: evs)
   borrowedFrom.replace(with: newBfi, context)
   return true
 }
 
+private func createEmptyBorrowedFrom(for phi: Phi, _ context: some MutatingContext) {
+  if let existingBfi = phi.borrowedFrom {
+    if existingBfi.enclosingValues.isEmpty {
+      return
+    }
+    existingBfi.replace(with: phi.value, context)
+  }
+  let builder = Builder(atBeginOf: phi.value.parentBlock, context)
+  let bfi = builder.createBorrowedFrom(borrowedValue: phi.value, enclosingValues: [])
+  phi.value.uses.ignoreUsers(ofType: BorrowedFromInst.self).replaceAll(with: bfi, context)
+}
+
 /// Replaces a phi with the unique incoming value if all incoming values are the same:
 /// ```
 ///   bb1:

SwiftCompilerSources/Sources/Optimizer/Utilities/Verifier.swift

@@ -96,6 +96,11 @@ private extension Phi {
 
 extension BorrowedFromInst : VerifiableInstruction {
   func verify(_ context: FunctionPassContext) {
+
+    for ev in enclosingValues {
+      require(ev.isValidEnclosingValueInBorrowedFrom, "invalid enclosing value in borrowed-from: \(ev)")
+    }
+
     var computedEVs = Stack<Value>(context)
     defer { computedEVs.deinitialize() }
 
@@ -114,6 +119,21 @@ extension BorrowedFromInst : VerifiableInstruction {
   }
 }
 
+private extension Value {
+  var isValidEnclosingValueInBorrowedFrom: Bool {
+    switch ownership {
+    case .owned:
+      return true
+    case .guaranteed:
+      return BeginBorrowValue(self) != nil ||
+             self is BorrowedFromInst ||
+             forwardingInstruction != nil
+    case .none, .unowned:
+      return false
+    }
+  }
+}
+
 extension LoadBorrowInst : VerifiableInstruction {
   func verify(_ context: FunctionPassContext) {
     if isUnchecked {

test/SILOptimizer/copy_propagation_borrow.sil

@@ -1209,7 +1209,7 @@ bb0(%0 : @guaranteed $C):
 }
 
 // CHECK-LABEL: sil [ossa] @test_updating_borrowed_from :
-// CHECK:         borrowed {{.*}} from {{.*}} %0 : $HasObject
+// CHECK:         borrowed {{.*}} from (%0 : $HasObject)
 // CHECK-LABEL: } // end sil function 'test_updating_borrowed_from'
 sil [ossa] @test_updating_borrowed_from : $@convention(thin) (@guaranteed HasObject) -> () {
 bb0(%0 : @guaranteed $HasObject):

test/SILOptimizer/copy_propagation_canonicalize_with_reborrows.sil

@@ -37,8 +37,8 @@ exit:
 
 // CHECK-LABEL: sil [ossa] @hoist_destroy_over_unrelated_endborrow : {{.*}} {
 // CHECK:       {{bb[0-9]+}}([[UNRELATED:%[^,]+]] : @reborrow $X, [[VALUE:%[^,]+]] : @owned $X):
-// CHECK:         destroy_value [[VALUE]]
 // CHECK:         [[BF:%.*]] = borrowed [[UNRELATED]] : $X from (%0 : $X)
+// CHECK:         destroy_value [[VALUE]]
 // CHECK:         end_borrow [[BF]]
 // CHECK-LABEL: } // end sil function 'hoist_destroy_over_unrelated_endborrow'
 sil [ossa] @hoist_destroy_over_unrelated_endborrow : $@convention(thin) (@guaranteed X) -> () {
@@ -90,8 +90,8 @@ exit:
 // CHECK-LABEL: sil [ossa] @hoist_destroy_over_unrelated_reborrow_reborrow_endborrow : {{.*}} {
 // CHECK:       {{bb[0-9]+}}({{%[^,]+}} : @reborrow $X, {{%[^,]+}} : @owned $X):
 // CHECK:       {{bb[0-9]+}}([[REBORROW:%[^,]+]] : @reborrow $X, [[VALUE:%[^,]+]] : @owned $X):
-// CHECK:         destroy_value [[VALUE]]
 // CHECK:         [[BF:%.*]] = borrowed [[REBORROW]] : $X from (%0 : $X)
+// CHECK:         destroy_value [[VALUE]]
 // CHECK:         end_borrow [[BF]]
 // CHECK-LABEL: } // end sil function 'hoist_destroy_over_unrelated_reborrow_reborrow_endborrow'
 sil [ossa] @hoist_destroy_over_unrelated_reborrow_reborrow_endborrow : $@convention(thin) (@guaranteed X) -> () {

test/SILOptimizer/semantic-arc-opts.sil

@@ -1612,3 +1612,32 @@ bb4(%4 : @owned $Klass):
   return %t : $()
 }
 
+// CHECK-LABEL: sil [ossa] @test_borrowed_from_updating : 
+// CHECK:       bb3({{.*}}):
+// CHECK-NEXT:    borrowed {{.*}} from (%0 : $FakeOptional<Klass>)
+// CHECK-NEXT:    borrowed {{.*}} from (%0 : $FakeOptional<Klass>)
+// CHECK-LABEL: } // end sil function 'test_borrowed_from_updating'
+sil [ossa] @test_borrowed_from_updating : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> () {
+bb0(%0 : @guaranteed $FakeOptional<Klass>):
+  %1 = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
+  %2 = begin_borrow %1
+  %3 = copy_value %0
+  switch_enum %3, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
+
+bb1(%5 : @owned $Klass):
+  end_borrow %2
+  %7 = enum $FakeOptional<Klass>, #FakeOptional.some!enumelt, %5
+  %8 = begin_borrow %7
+  br bb3(%8, %7)
+
+bb2:
+  br bb3(%2, %1)
+
+bb3(%11 : @reborrow $FakeOptional<Klass>, %12 : @owned $FakeOptional<Klass>):
+  %13 = borrowed %11 from (%12)
+  end_borrow %13
+  destroy_value %12
+  %16 = tuple ()
+  return %16
+}
+

test/SILOptimizer/sil_combine_misc_opts.sil

@@ -102,7 +102,7 @@ bb0(%0 : $*Builtin.NativeObject):
 }
 
 // CHECK-LABEL: sil [ossa] @test_updating_borrowed_from :
-// CHECK:         borrowed {{.*}} from {{.*}} %0 : $S
+// CHECK:         borrowed {{.*}} from (%0 : $S)
 // CHECK-LABEL: } // end sil function 'test_updating_borrowed_from'
 sil [ossa] @test_updating_borrowed_from : $@convention(thin) (@guaranteed S) -> () {
 bb0(%0 : @guaranteed $S):