Use PrunedLiveness in LoadBorrowImmutabilityChecker instead of LinearLifetimeChecker

Author: meg-gupta

lib/SIL/Verifier/LoadBorrowImmutabilityChecker.cpp

@@ -27,6 +27,7 @@
 #include "swift/SIL/LinearLifetimeChecker.h"
 #include "swift/SIL/MemAccessUtils.h"
 #include "swift/SIL/OwnershipUtils.h"
+#include "swift/SIL/PrunedLiveness.h"
 #include "swift/SIL/Projection.h"
 #include "swift/SIL/SILInstruction.h"
 #include "llvm/Support/Debug.h"
@@ -302,7 +303,7 @@ LoadBorrowImmutabilityAnalysis::LoadBorrowImmutabilityAnalysis(
 
 // \p address may be an address, pointer, or box type.
 bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
-    LoadBorrowInst *lbi, ArrayRef<Operand *> endBorrowUses,
+    LoadBorrowInst *lbi,
     AccessPathWithBase accessPathWithBase) {
   auto accessPath = accessPathWithBase.accessPath;
   LinearLifetimeChecker checker(deadEndBlocks);
@@ -317,25 +318,29 @@ bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
   auto ownershipRoot = accessPath.getStorage().isReference()
                            ? findOwnershipReferenceRoot(accessPathWithBase.base)
                            : SILValue();
+
+  BorrowedValue borrowedValue(lbi);
+  PrunedLiveness borrowLiveness;
+  borrowedValue.computeLiveness(borrowLiveness);
+
   // Then for each write...
   for (auto *op : *writes) {
+    auto *write = op->getUser();
     // First see if the write is a dead end block. In such a case, just skip it.
-    if (deadEndBlocks.isDeadEnd(op->getUser()->getParent())) {
+    if (deadEndBlocks.isDeadEnd(write->getParent())) {
       continue;
     }
     // A destroy_value will be a definite write only when the destroy is on the
     // ownershipRoot
-    if (isa<DestroyValueInst>(op->getUser())) {
+    if (isa<DestroyValueInst>(write)) {
       if (op->get() != ownershipRoot)
         continue;
     }
-    // See if the write is within the load borrow's lifetime. If it isn't, we
-    // don't have to worry about it.
-    if (!checker.validateLifetime(lbi, endBorrowUses, op)) {
-      continue;
+
+    if (borrowLiveness.isWithinBoundaryOfDef(write, lbi)) {
+      llvm::errs() << "Write: " << *write;
+      return false;
     }
-    llvm::errs() << "Write: " << *op->getUser();
-    return false;
   }
   // Ok, we are good.
   return true;
@@ -359,13 +364,6 @@ bool LoadBorrowImmutabilityAnalysis::isImmutable(LoadBorrowInst *lbi) {
   if (accessPath.getStorage().isLetAccess()) {
     return true;
   }
-  // At this point, we know that we /may/ have writes. Now we go through various
-  // cases to try and exhaustively identify if those writes overlap with our
-  // load_borrow.
-  SmallVector<Operand *, 8> endBorrowUses;
-  visitTransitiveEndBorrows(lbi, [&](EndBorrowInst *endBorrow) {
-    endBorrowUses.push_back(&endBorrow->getOperandRef());
-  });
 
   switch (accessPath.getStorage().getKind()) {
   case AccessStorage::Nested: {
@@ -380,15 +378,15 @@ bool LoadBorrowImmutabilityAnalysis::isImmutable(LoadBorrowInst *lbi) {
     //
     // TODO: As a separate analysis, verify that the load_borrow scope is always
     // nested within the begin_access scope (to ensure no aliasing access).
-    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
+    return isImmutableInScope(lbi, accessPathWithBase);
   }
   case AccessStorage::Argument: {
     auto *arg =
         cast<SILFunctionArgument>(accessPath.getStorage().getArgument());
     if (arg->hasConvention(SILArgumentConvention::Indirect_In_Guaranteed)) {
       return true;
     }
-    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
+    return isImmutableInScope(lbi, accessPathWithBase);
   }
   // FIXME: A yielded address could overlap with another in this function.
   case AccessStorage::Yield:
@@ -398,7 +396,7 @@ bool LoadBorrowImmutabilityAnalysis::isImmutable(LoadBorrowInst *lbi) {
   case AccessStorage::Tail:
   case AccessStorage::Global:
   case AccessStorage::Unidentified:
-    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
+    return isImmutableInScope(lbi, accessPathWithBase);
   }
   llvm_unreachable("Covered switch isn't covered?!");
 }

lib/SIL/Verifier/VerifierPrivate.h

@@ -40,7 +40,6 @@ class LoadBorrowImmutabilityAnalysis {
 
 private:
   bool isImmutableInScope(LoadBorrowInst *lbi,
-                          ArrayRef<Operand *> endBorrowUses,
                           AccessPathWithBase accessPathWithBase);
 };
 

test/SIL/OwnershipVerifier/load_borrow_verify.sil

@@ -11,6 +11,11 @@ sil_stage canonical
 import Builtin
 import Swift
 
+public enum FakeOptional<T> {
+  case none
+  case some(T)
+}
+
 private var testGlobal: Int64
 
 sil_global private @globalinit_33_00F4D2139E6BDDFEC71E5005B67B5674_token0 : $Builtin.Word
@@ -173,3 +178,82 @@ bb2(%ld : @guaranteed $K):
   return %6 : $()
 }
 
+sil [ossa] @test_multiple_loadborrows : $@convention(thin) (@owned K, @owned K) -> () {
+ bb0(%0 : @owned $K, %1 : @owned $K):
+   %stk = alloc_stack [lexical] $K
+   cond_br undef, bb1, bb2
+
+ bb1:
+   destroy_value %1 : $K
+   store %0 to [init] %stk : $*K
+   br bb3
+
+ bb2:
+   destroy_value %0 : $K
+   store %1 to [init] %stk : $*K
+   br bb3
+
+ bb3:
+   cond_br undef, bb4, bb5
+
+ bb4:
+   %ld1 = load_borrow %stk : $*K
+   br bb6(%ld1 : $K)
+
+ bb5:
+   %ld2 = load_borrow %stk : $*K
+   br bb6(%ld2 : $K)
+
+ bb6(%ld : @guaranteed $K):
+   %3 = function_ref @use_guaranteed : $@convention(thin) (@guaranteed K) -> ()
+   %4 = apply %3(%ld) : $@convention(thin) (@guaranteed K) -> ()
+   end_borrow %ld : $K
+   destroy_addr %stk : $*K
+   dealloc_stack %stk : $*K
+   %6 = tuple ()
+   return %6 : $()
+ }
+
+sil [ossa] @test_multi_bb : $@convention(thin) (@inout FakeOptional<K>) -> () {
+bb0(%0 : $*FakeOptional<K>):
+  %1 = begin_access [modify] [static] %0 : $*FakeOptional<K>
+  %2 = load_borrow %1 : $*FakeOptional<K>
+  %4 = copy_value %2 : $FakeOptional<K>
+  switch_enum %4 : $FakeOptional<K>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
+
+bb1(%6 : @owned $K):
+  destroy_value %6 : $K
+  end_borrow %2 : $FakeOptional<K>
+  end_access %1 : $*FakeOptional<K>
+  br bb3
+
+bb2:
+  unreachable
+
+bb3:
+  %12 = tuple ()
+  return %12 : $()
+}
+
+sil [ossa] @test_write_reborrow_loop : $@convention(thin) (@owned K) -> () {
+bb0(%0 : @owned $K):
+  %stk = alloc_stack $K
+  store %0 to [init] %stk : $*K
+  %ld1 = load_borrow %stk : $*K
+  br bb1(%ld1 : $K)
+
+bb1(%ld2 : @guaranteed $K):
+  cond_br undef, bb2, bb3
+
+bb2:
+  %3 = function_ref @use_guaranteed : $@convention(thin) (@guaranteed K) -> ()
+  %4 = apply %3(%ld2) : $@convention(thin) (@guaranteed K) -> ()
+  br bb1(%ld2 : $K)
+
+bb3:
+  end_borrow %ld2 : $K
+  destroy_addr %stk : $*K
+  dealloc_stack %stk : $*K
+  %9999 = tuple()
+  return %9999 : $()
+}

test/SIL/OwnershipVerifier/load_borrow_verify_errors.sil

@@ -25,3 +25,49 @@ bb2(%ld : @guaranteed $Klass):
   %6 = tuple ()
   return %6 : $()
 }
+
+// CHECK: Write:   destroy_addr %2 : $*Klass
+// CHECK: Begin Error in function test_multiple_loadborrows
+// CHECK: SIL verification failed: Found load borrow that is invalidated by a local write?!: loadBorrowImmutabilityAnalysis.isImmutable(LBI)
+// CHECK: Verifying instruction:
+// CHECK:      %2 = alloc_stack [lexical] $Klass
+// CHECK: ->   %14 = load_borrow %2 : $*Klass
+// CHECK:      br bb6(%14 : $Klass)
+// CHECK: End Error in function test_multiple_loadborrows
+sil [ossa] @test_multiple_loadborrows : $@convention(thin) (@owned Klass, @owned Klass) -> () {
+bb0(%0 : @owned $Klass, %1 : @owned $Klass):
+  %stk = alloc_stack [lexical] $Klass
+  cond_br undef, bb1, bb2
+
+bb1:
+  destroy_value %1 : $Klass
+  store %0 to [init] %stk : $*Klass
+  br bb3
+
+bb2:
+  destroy_value %0 : $Klass
+  store %1 to [init] %stk : $*Klass
+  br bb3
+
+bb3:
+  cond_br undef, bb4, bb5
+
+bb4:
+  %ld1 = load_borrow %stk : $*Klass
+  destroy_addr %stk : $*Klass
+  br bb6(%ld1 : $Klass)
+
+bb5:
+  %ld2 = load_borrow %stk : $*Klass
+  destroy_addr %stk : $*Klass
+  br bb6(%ld2 : $Klass)
+
+bb6(%ld : @guaranteed $Klass):
+  %3 = function_ref @use_guaranteed : $@convention(thin) (@guaranteed Klass) -> ()
+  %4 = apply %3(%ld) : $@convention(thin) (@guaranteed Klass) -> ()
+  end_borrow %ld : $Klass
+  dealloc_stack %stk : $*Klass
+  %6 = tuple ()
+  return %6 : $()
+}
+