Merge pull request #62573 from meg-gupta/sbiverify

Update store_borrow checking in MemoryLifetimeVerifier and fix ForEachLoopUnroll and GenericCloner

Author: meg-gupta

include/swift/SIL/SILInstruction.h

@@ -4368,47 +4368,7 @@ class EndBorrowInst
   EndBorrowInst(SILDebugLocation debugLoc, SILValue borrowedValue)
       : UnaryInstructionBase(debugLoc, borrowedValue) {}
 
-public:
-  /// Return the value that this end_borrow is ending the borrow of if we are
-  /// borrowing a single value.
-  SILValue getSingleOriginalValue() const {
-    SILValue v = getOperand();
-    if (auto *bbi = dyn_cast<BeginBorrowInst>(v))
-      return bbi->getOperand();
-    if (auto *lbi = dyn_cast<LoadBorrowInst>(v))
-      return lbi->getOperand();
-    return SILValue();
-  }
 
-  /// Return the set of guaranteed values that have scopes ended by this
-  /// end_borrow.
-  ///
-  /// Discussion: We can only have multiple values associated with an end_borrow
-  /// in the case of having Phi arguments with guaranteed inputs. This is
-  /// necessary to represent certain conditional operations such as:
-  ///
-  /// class Klass {
-  ///   let k1: Klass
-  ///   let k2: Klass
-  /// }
-  ///
-  /// func useKlass(k: Klass) { ... }
-  /// var boolValue : Bool { ... }
-  ///
-  /// func f(k: Klass) {
-  ///   useKlass(boolValue ? k.k1 : k.k2)
-  /// }
-  ///
-  /// Today, when we SILGen such code, we copy k.k1 and k.k2 before the Phi when
-  /// it could potentially be avoided. So today this just appends
-  /// getSingleOriginalValue() to originalValues.
-  ///
-  /// TODO: Once this changes, this code must be update.
-  void getOriginalValues(SmallVectorImpl<SILValue> &originalValues) const {
-    SILValue value = getSingleOriginalValue();
-    assert(value && "Guaranteed phi arguments are not supported now");
-    originalValues.emplace_back(value);
-  }
 };
 
 /// Different kinds of access.

lib/IRGen/IRGenModule.cpp

@@ -1871,6 +1871,7 @@ bool IRGenModule::canMakeStaticObjectsReadOnly() {
   // rdar://101126543
   return false;
 
+#if 0
   if (getOptions().DisableReadonlyStaticObjects)
     return false;
 
@@ -1881,6 +1882,7 @@ bool IRGenModule::canMakeStaticObjectsReadOnly() {
 
   return getAvailabilityContext().isContainedIn(
           Context.getImmortalRefCountSymbolsAvailability());
+#endif
 }
 
 void IRGenerator::addGenModule(SourceFile *SF, IRGenModule *IGM) {

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -377,6 +377,13 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
         }
         break;
       }
+      case SILInstructionKind::EndBorrowInst: {
+        auto *ebi = cast<EndBorrowInst>(&I);
+        if (auto *sbi = dyn_cast<StoreBorrowInst>(ebi->getOperand())) {
+          killBits(state, sbi->getDest());
+        }
+        break;
+      }
       case SILInstructionKind::DestroyAddrInst:
       case SILInstructionKind::DeallocStackInst:
         killBits(state, I.getOperand(0));
@@ -696,8 +703,13 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         break;
       }
       case SILInstructionKind::EndBorrowInst: {
-        if (SILValue orig = cast<EndBorrowInst>(&I)->getSingleOriginalValue())
-          requireBitsSet(bits, orig, &I);
+        auto *ebi = cast<EndBorrowInst>(&I);
+        if (auto *sbi = dyn_cast<StoreBorrowInst>(ebi->getOperand())) {
+          requireBitsSet(bits, sbi->getDest(), &I);
+          locations.clearBits(bits, sbi->getDest());
+        } else if (auto *lbi = dyn_cast<LoadBorrowInst>(ebi->getOperand())) {
+          requireBitsSet(bits, lbi->getOperand(), &I);
+        }
         break;
       }
       case SILInstructionKind::UncheckedRefCastAddrInst:
@@ -779,11 +791,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
       }
       case SILInstructionKind::DeallocStackInst: {
         SILValue opVal = cast<DeallocStackInst>(&I)->getOperand();
-        if (isStoreBorrowLocation(opVal)) {
-          requireBitsSet(bits, opVal, &I);
-        } else {
-          requireBitsClear(bits & nonTrivialLocations, opVal, &I);
-        }
+        requireBitsClear(bits & nonTrivialLocations, opVal, &I);
         // Needed to clear any bits of trivial locations (which are not required
         // to be zero).
         locations.clearBits(bits, opVal);

lib/SILOptimizer/LoopTransforms/ForEachLoopUnroll.cpp

@@ -538,6 +538,7 @@ static void unrollForEach(ArrayInfo &arrayInfo, TryApplyInst *forEachCall,
     SILBasicBlock *currentBB = num > 1 ? normalTargetGenerator(nextNormalBB)
                                        : forEachCall->getParentBlock();
     SILBuilderWithScope unrollBuilder(currentBB, forEachCall);
+    SILBuilderWithScope normalBuilder(&nextNormalBB->front(), forEachCall);
     SILValue borrowedElem;
     SILValue addr;
 
@@ -552,9 +553,8 @@ static void unrollForEach(ArrayInfo &arrayInfo, TryApplyInst *forEachCall,
       borrowedElem = unrollBuilder.createBeginBorrow(forEachLoc, elementCopy);
       addr =
           unrollBuilder.createStoreBorrow(forEachLoc, borrowedElem, allocStack);
-      SILBuilderWithScope builder(&nextNormalBB->front(), forEachCall);
-      builder.createEndBorrow(forEachLoc, addr);
-      builder.createEndBorrow(forEachLoc, borrowedElem);
+      normalBuilder.createEndBorrow(forEachLoc, addr);
+      normalBuilder.createEndBorrow(forEachLoc, borrowedElem);
     }
 
     SILBasicBlock *errorTarget =
@@ -566,16 +566,18 @@ static void unrollForEach(ArrayInfo &arrayInfo, TryApplyInst *forEachCall,
     unrollBuilder.createTryApply(forEachLoc, forEachBodyClosure,
                                  SubstitutionMap(), addr, nextNormalBB,
                                  errorTarget);
+
+    if (nextNormalBB == normalBB) {
+      // Dealloc the stack in the normalBB and also in errorBB. Note that every
+      // try_apply created during the unrolling must pass through these blocks.
+      normalBuilder.createDeallocStack(forEachLoc, allocStack);
+    }
     nextNormalBB = currentBB;
   }
-
   // Dealloc the stack in the normalBB and also in errorBB. Note that every
   // try_apply created during the unrolling must pass through these blocks.
-  SILBuilderWithScope(&normalBB->front())
-      .createDeallocStack(forEachLoc, allocStack);
   SILBuilderWithScope(&errorBB->front())
       .createDeallocStack(forEachLoc, allocStack);
-
   // Remove the forEach call as it has now been unrolled.
   removeForEachCall(forEachCall, deleter);
 }

lib/SILOptimizer/Utils/GenericCloner.cpp

@@ -255,8 +255,9 @@ void GenericCloner::postFixUp(SILFunction *f) {
       scopedAddress.endScopeAtLivenessBoundary(&storeBorrowLiveness);
       continue;
     }
-    for (auto *exit : FunctionExits) {
-      scopedAddress.createScopeEnd(exit->getIterator(),
+    auto *alloc = cast<AllocStackInst>(sbi->getDest());
+    for (auto *dealloc : alloc->getUsersOfType<DeallocStackInst>()) {
+      scopedAddress.createScopeEnd(dealloc->getIterator(),
                                    RegularLocation::getAutoGeneratedLocation());
     }
   }

test/SIL/memory_lifetime_failures.sil

@@ -373,6 +373,17 @@ bb0(%0 :  @guaranteed $Optional<T>):
   return %res : $()
 }
 
+// CHECK: SIL memory lifetime failure in @test_store_borrow_addr_after_dealloc: memory is initialized, but shouldn't be
+sil [ossa] @test_store_borrow_addr_after_dealloc : $@convention(thin) (@guaranteed Optional<T>) -> () {
+bb0(%0 :  @guaranteed $Optional<T>):
+  %s = alloc_stack $Optional<T>
+  %sb = store_borrow %0 to %s : $*Optional<T>
+  dealloc_stack %s : $*Optional<T>
+  end_borrow %sb : $*Optional<T>
+  %res = tuple ()
+  return %res : $()
+}
+
 // CHECK: SIL memory lifetime failure in @test_cast_br_take_always: memory is not initialized, but should be
 sil [ossa] @test_cast_br_take_always : $@convention(thin) <U, V> (@in U) -> () {
 bb0(%0 : $*U):

test/SIL/store_borrow_verify_errors.sil

@@ -1,4 +1,7 @@
-// RUN: %target-sil-opt -verify-continue-on-failure -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s 
+// RUN: %target-sil-opt -dont-abort-on-memory-lifetime-errors -verify-continue-on-failure -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s 
+
+// Memory lifetime verifier also raises errors in some of these cases, add dont-abort-on-memory-lifetime-errors so that we don't check it's output.
+// Memory lifetime verifier cannot subsume this because the verification is disabled on unreachable paths.
 
 import Builtin
 

test/SILOptimizer/for_each_loop_unroll_test.sil

@@ -134,8 +134,8 @@ bb2(%39 : @owned $Error):
 // CHECK: br [[ERROR3:bb[0-9]+]]([[ERRPARAM2]] : $any Error)
 
 // CHECK: [[NORMAL2]](%{{.*}} : $()):
-// CHECK: dealloc_stack [[STACK]]
 // CHECK: end_borrow [[ELEM2BORROW]]
+// CHECK: dealloc_stack [[STACK]]
 // Note that the temporary alloc_stack of the array created for the forEach call
 // will be cleaned up when the forEach call is removed.
 // CHECK: destroy_value

test/SILOptimizer/specialize_ossa.sil

@@ -32,6 +32,14 @@ bb0(%0 : $Builtin.NativeObject):
   return %9999 : $()
 }
 
+// CHECK-LABEL: sil shared [noinline] [ossa] @$s33XXX_foo_guaranteed_generic_returnBo_Tg5 :
+// CHECK:  [[S1:%.*]] = alloc_stack $Builtin.NativeObject
+// CHECK:  [[S2:%.*]] = alloc_stack $Builtin.NativeObject
+// CHECK:  [[SBI:%.*]] = store_borrow %0 to [[S2]] : $*Builtin.NativeObject
+// CHECK:  end_borrow [[SBI]] : $*Builtin.NativeObject
+// CHECK:  dealloc_stack [[S2]] : $*Builtin.NativeObject
+// CHECK-LABEL: } // end sil function '$s33XXX_foo_guaranteed_generic_returnBo_Tg5'
+
 // CHECK-LABEL: sil [ossa] @exp1 : $@convention(thin) () -> () {
 // CHECK-NOT: apply
 // Call of specialized initializer: <Int32>
@@ -115,7 +123,6 @@ bb0(%0 : $*T, %1 : $*XXX<T>):
   return %9 : $Int32                              // id: %11
 }
 
-
 sil [ossa] [noinline] @XXX_foo_guaranteed_generic_return : $@convention(method) <T> (@in_guaranteed T, @in XXX<T>) -> @out T {
 bb0(%0 : $*T, %1 : $*T, %2 : $*XXX<T>):
   %3 = address_to_pointer %1 : $*T to $Builtin.RawPointer