Merge pull request #64554 from meg-gupta/fixsilcombineubci

Fix OSSA RAUW utility's insertion point

Author: meg-gupta

lib/SILOptimizer/SILCombiner/SILCombinerCastVisitors.cpp

@@ -337,9 +337,8 @@ visitPointerToAddressInst(PointerToAddressInst *PTAI) {
       OwnershipRAUWHelper helper(ownershipFixupContext, PTAI,
                                  ATPI->getOperand());
       if (helper) {
-        SILBuilderWithScope localBuilder(std::next(PTAI->getIterator()), Builder);
         auto replacement = helper.prepareReplacement();
-        auto *newInst = localBuilder.createUncheckedAddrCast(
+        auto *newInst = Builder.createUncheckedAddrCast(
             PTAI->getLoc(), replacement, PTAI->getType());
         helper.perform(newInst);
         return nullptr;
@@ -818,15 +817,11 @@ SILCombiner::visitRawPointerToRefInst(RawPointerToRefInst *rawToRef) {
       SILValue originalRef = refToRaw->getOperand();
       OwnershipRAUWHelper helper(ownershipFixupContext, rawToRef, originalRef);
       if (helper) {
-        // We use the refToRaw's insertion point to insert our
-        // unchecked_ref_cast, since we don't know if our guaranteed value
-        SILBuilderWithScope localBuilder(std::next(refToRaw->getIterator()),
-                                         Builder);
         // Since we are using std::next, we use getAutogeneratedLocation to
         // avoid any issues if our next insertion point is a terminator.
         auto loc = RegularLocation::getAutoGeneratedLocation();
         auto replacement = helper.prepareReplacement();
-        auto *newInst = localBuilder.createUncheckedRefCast(
+        auto *newInst = Builder.createUncheckedRefCast(
             loc, replacement, rawToRef->getType());
         // If we have an operand with ownership, we need to change our
         // unchecked_ref_cast to produce an unowned value. This is because

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

@@ -943,23 +943,9 @@ BeginBorrowInst *OwnershipLifetimeExtender::borrowCopyOverGuaranteedUses(
                                        makeUserRange(guaranteedUsePoints));
 }
 
-// Return the borrow position when replacing guaranteedValue with newValue.
-//
-// Precondition: newValue's block dominates and reaches guaranteedValue's block.
-//
-// Postcondition: The returned instruction's block is guaranteedValue's block.
-//
-// If \p newValue and \p guaranteedValue are in the same block, borrow at the
-// newValue just in case it is defined later in the block (to avoid scanning
-// instructions). Otherwise, borrow in the guaranteedValue's block to avoid
-// introducing the borrow scope too early--not only would this require extra
-// cleanup, but it would hinder optimization.
-static SILBasicBlock::iterator getBorrowPoint(SILValue newValue,
-                                              SILValue guaranteedValue) {
-  if (newValue->getParentBlock() == guaranteedValue->getParentBlock())
-    return newValue->getNextInstruction()->getIterator();
-
-  return guaranteedValue->getNextInstruction()->getIterator();
+// Return the borrow position when replacing oldValue.
+static SILBasicBlock::iterator getBorrowPoint(SILValue oldValue) {
+  return oldValue->getDefiningInsertionPoint()->getIterator();
 }
 
 /// Borrow \p newValue over the lifetime of \p guaranteedValue. Return the
@@ -991,7 +977,7 @@ OwnershipLifetimeExtender::borrowOverValue(SILValue newValue,
     return newValue;
 
   // FIXME: use GuaranteedOwnershipExtension
-  auto borrowPt = getBorrowPoint(newValue, guaranteedValue);
+  auto borrowPt = getBorrowPoint(guaranteedValue);
   return borrowCopyOverGuaranteedUses(
       newValue, borrowPt, ArrayRef<Operand *>(ctx.guaranteedUsePoints));
 }
@@ -1151,7 +1137,7 @@ SILValue OwnershipRAUWPrepare::prepareUnowned(SILValue newValue) {
     }
 
     auto extender = getLifetimeExtender();
-    auto borrowPt = getBorrowPoint(newValue, oldValue);
+    auto borrowPt = getBorrowPoint(oldValue);
     SILValue borrow = extender.borrowCopyOverGuaranteedUses(
         newValue, borrowPt, oldValue->getUses());
     return borrow;
@@ -1267,7 +1253,7 @@ OwnershipRAUWHelper::getReplacementAddress() {
   // value. Then we RAUW as appropriate.
   OwnershipLifetimeExtender extender{*ctx};
   auto base = ctx->extraAddressFixupInfo.base;
-  auto borrowPt = getBorrowPoint(newValue, oldValue);
+  auto borrowPt = getBorrowPoint(oldValue);
   // FIXME: why does this use allAddressUsesFromOldValue instead of
   // guaranteedUsePoints?
   BeginBorrowInst *bbi = extender.borrowCopyOverGuaranteedUses(

test/SILOptimizer/sil_combine_nocanonicalize_ossa.sil

@@ -5,6 +5,7 @@
 
 sil_stage canonical
 
+import Swift
 import Builtin
 
 class Klass {}
@@ -81,3 +82,25 @@ bb0:
   destroy_value %obj : $KlassWithTailAllocatedElems
   return %val : $Builtin.Word
 }
+
+sil @get_native_object : $@convention(thin) () -> @owned Builtin.NativeObject
+
+// CHECK-LABEL: sil [ossa] @bitwise_combines_guaranteed :
+// CHECK: unchecked_ref_cast
+// CHECK-LABEL: } // end sil function 'bitwise_combines_guaranteed'
+sil [ossa] @bitwise_combines_guaranteed : $@convention(thin) () -> @owned Optional<Builtin.NativeObject> {
+bb0:
+  %f = function_ref @get_native_object : $@convention(thin) () -> @owned Builtin.NativeObject
+  %0 = apply %f() : $@convention(thin) () -> @owned Builtin.NativeObject
+  %b = begin_borrow [lexical] %0 : $Builtin.NativeObject
+  br bb1
+
+bb1:
+  %2 = unchecked_trivial_bit_cast %b : $Builtin.NativeObject to $Builtin.Word
+  %3 = unchecked_bitwise_cast %2 : $Builtin.Word to $Optional<Builtin.NativeObject>
+  %c = copy_value %3 : $Optional<Builtin.NativeObject>
+  end_borrow %b : $Builtin.NativeObject
+  destroy_value %0 : $Builtin.NativeObject
+  return %c : $Optional<Builtin.NativeObject>
+}
+