Fix LoadBorrowImmutabilityAnalysis in cases it raises false errors on destroy_value

We should only flag an error if there is a destroy_value on the first owned root of the access path.

Author: meg-gupta

include/swift/SIL/MemAccessUtils.h

@@ -171,6 +171,9 @@ SILValue getAccessBase(SILValue address);
 /// AccessedStorage::getRoot() and AccessPath::getRoot().
 SILValue findReferenceRoot(SILValue ref);
 
+/// Find the first owned root of the reference.
+SILValue findOwnershipReferenceRoot(SILValue ref);
+
 /// Return true if \p address points to a let-variable.
 ///
 /// let-variables are only written during let-variable initialization, which is
@@ -1264,9 +1267,13 @@ SILBasicBlock::iterator removeBeginAccess(BeginAccessInst *beginAccess);
 
 namespace swift {
 
-/// Return true if \p svi is a cast that preserves the identity and
+/// Return true if \p svi is a cast that preserves the identity equivalence of
+/// the reference at operand zero.
+bool isIdentityPreservingRefCast(SingleValueInstruction *svi);
+
+/// Return true if \p svi is a cast that preserves the identity equivalence and
 /// reference-counting equivalence of the reference at operand zero.
-bool isRCIdentityPreservingCast(SingleValueInstruction *svi);
+bool isIdentityAndOwnershipPreservingRefCast(SingleValueInstruction *svi);
 
 /// If \p svi is an access projection, return an address-type operand for the
 /// incoming address.

lib/SIL/Utils/InstructionUtils.cpp

@@ -125,10 +125,9 @@ SILValue swift::stripCastsWithoutMarkDependence(SILValue v) {
       return v;
 
     if (auto *svi = dyn_cast<SingleValueInstruction>(v)) {
-      if (isRCIdentityPreservingCast(svi)
-          || isa<UncheckedTrivialBitCastInst>(v)
-          || isa<BeginAccessInst>(v)
-          || isa<EndCOWMutationInst>(v)) {
+      if (isIdentityPreservingRefCast(svi) ||
+          isa<UncheckedTrivialBitCastInst>(v) || isa<BeginAccessInst>(v) ||
+          isa<EndCOWMutationInst>(v)) {
         v = svi->getOperand(0);
         continue;
       }
@@ -141,10 +140,9 @@ SILValue swift::stripCasts(SILValue v) {
   while (true) {
     v = stripSinglePredecessorArgs(v);
     if (auto *svi = dyn_cast<SingleValueInstruction>(v)) {
-      if (isRCIdentityPreservingCast(svi)
-          || isa<UncheckedTrivialBitCastInst>(v)
-          || isa<MarkDependenceInst>(v)
-          || isa<BeginAccessInst>(v)) {
+      if (isIdentityPreservingRefCast(svi) ||
+          isa<UncheckedTrivialBitCastInst>(v) || isa<MarkDependenceInst>(v) ||
+          isa<BeginAccessInst>(v)) {
         v = cast<SingleValueInstruction>(v)->getOperand(0);
         continue;
       }

lib/SIL/Utils/MemAccessUtils.cpp

@@ -365,6 +365,12 @@ bool swift::isLetAddress(SILValue address) {
 //                          MARK: FindReferenceRoot
 //===----------------------------------------------------------------------===//
 
+bool swift::isIdentityPreservingRefCast(SingleValueInstruction *svi) {
+  // Ignore both copies and other identity and ownership preserving casts
+  return isa<CopyValueInst>(svi) ||
+         isIdentityAndOwnershipPreservingRefCast(svi);
+}
+
 // On some platforms, casting from a metatype to a reference type dynamically
 // allocates a ref-counted box for the metatype. Naturally that is the place
 // where RC-identity begins. Considering the source of such a casts to be
@@ -374,12 +380,12 @@ bool swift::isLetAddress(SILValue address) {
 // The SILVerifier checks that none of these operations cast a trivial value to
 // a reference except unconditional_checked_cast[_value], which is checked By
 // SILDynamicCastInst::isRCIdentityPreserving().
-bool swift::isRCIdentityPreservingCast(SingleValueInstruction *svi) {
+bool swift::isIdentityAndOwnershipPreservingRefCast(
+    SingleValueInstruction *svi) {
   switch (svi->getKind()) {
   default:
     return false;
-  // Ignore ownership casts
-  case SILInstructionKind::CopyValueInst:
+  // Ignore borrows
   case SILInstructionKind::BeginBorrowInst:
   // Ignore class type casts
   case SILInstructionKind::UpcastInst:
@@ -402,8 +408,12 @@ namespace {
 // Essentially RC identity where the starting point is already a reference.
 class FindReferenceRoot {
   SmallPtrSet<SILPhiArgument *, 4> visitedPhis;
+  bool preserveOwnership;
 
 public:
+  FindReferenceRoot(bool preserveOwnership)
+      : preserveOwnership(preserveOwnership) {}
+
   SILValue findRoot(SILValue ref) && {
     SILValue root = recursiveFindRoot(ref);
     assert(root && "all phi inputs must be reachable");
@@ -414,8 +424,15 @@ class FindReferenceRoot {
   // Return an invalid value for a phi with no resolved inputs.
   SILValue recursiveFindRoot(SILValue ref) {
     while (auto *svi = dyn_cast<SingleValueInstruction>(ref)) {
-      if (!isRCIdentityPreservingCast(svi)) {
-        break;
+      // If preserveOwnership is true, stop at the first owned root
+      if (preserveOwnership) {
+        if (!isIdentityAndOwnershipPreservingRefCast(svi)) {
+          break;
+        }
+      } else {
+        if (!isIdentityPreservingRefCast(svi)) {
+          break;
+        }
       }
       ref = svi->getOperand(0);
     };
@@ -451,7 +468,11 @@ class FindReferenceRoot {
 } // end anonymous namespace
 
 SILValue swift::findReferenceRoot(SILValue ref) {
-  return FindReferenceRoot().findRoot(ref);
+  return FindReferenceRoot(false /*preserveOwnership*/).findRoot(ref);
+}
+
+SILValue swift::findOwnershipReferenceRoot(SILValue ref) {
+  return FindReferenceRoot(true /*preserveOwnership*/).findRoot(ref);
 }
 
 //===----------------------------------------------------------------------===//
@@ -1340,7 +1361,7 @@ AccessPathDefUseTraversal::UseKind
 AccessPathDefUseTraversal::visitSingleValueUser(SingleValueInstruction *svi,
                                                 DFSEntry dfs) {
   if (dfs.isRef()) {
-    if (isRCIdentityPreservingCast(svi)) {
+    if (isIdentityPreservingRefCast(svi)) {
       pushUsers(svi, dfs);
       return IgnoredUse;
     }

lib/SIL/Verifier/LoadBorrowImmutabilityChecker.cpp

@@ -292,8 +292,8 @@ LoadBorrowImmutabilityAnalysis::LoadBorrowImmutabilityAnalysis(
 // \p address may be an address, pointer, or box type.
 bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
     LoadBorrowInst *lbi, ArrayRef<Operand *> endBorrowUses,
-    AccessPath accessPath) {
-
+    AccessPathWithBase accessPathWithBase) {
+  auto accessPath = accessPathWithBase.accessPath;
   LinearLifetimeChecker checker(deadEndBlocks);
   auto writes = cache.get(accessPath);
 
@@ -303,12 +303,21 @@ bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
     accessPath.getStorage().print(llvm::errs());
     return false;
   }
+  auto ownershipRoot = accessPath.getStorage().isReference()
+                           ? findOwnershipReferenceRoot(accessPathWithBase.base)
+                           : SILValue();
   // Then for each write...
   for (auto *op : *writes) {
     // First see if the write is a dead end block. In such a case, just skip it.
     if (deadEndBlocks.isDeadEnd(op->getUser()->getParent())) {
       continue;
     }
+    // A destroy_value will be a definite write only when the destroy is on the
+    // ownershipRoot
+    if (isa<DestroyValueInst>(op->getUser())) {
+      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)) {
@@ -326,7 +335,9 @@ bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
 //===----------------------------------------------------------------------===//
 
 bool LoadBorrowImmutabilityAnalysis::isImmutable(LoadBorrowInst *lbi) {
-  AccessPath accessPath = AccessPath::computeInScope(lbi->getOperand());
+  auto accessPathWithBase = AccessPathWithBase::compute(lbi->getOperand());
+  auto accessPath = accessPathWithBase.accessPath;
+
   // Bail on an invalid AccessPath. AccessPath completeness is verified
   // independently--it may be invalid in extraordinary situations. When
   // AccessPath is valid, we know all its uses are recognizable.
@@ -358,15 +369,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, accessPath);
+    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
   }
   case AccessedStorage::Argument: {
     auto *arg =
         cast<SILFunctionArgument>(accessPath.getStorage().getArgument());
     if (arg->hasConvention(SILArgumentConvention::Indirect_In_Guaranteed)) {
       return true;
     }
-    return isImmutableInScope(lbi, endBorrowUses, accessPath);
+    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
   }
   // FIXME: A yielded address could overlap with another in this function.
   case AccessedStorage::Yield:
@@ -376,7 +387,7 @@ bool LoadBorrowImmutabilityAnalysis::isImmutable(LoadBorrowInst *lbi) {
   case AccessedStorage::Tail:
   case AccessedStorage::Global:
   case AccessedStorage::Unidentified:
-    return isImmutableInScope(lbi, endBorrowUses, accessPath);
+    return isImmutableInScope(lbi, endBorrowUses, accessPathWithBase);
   }
   llvm_unreachable("Covered switch isn't covered?!");
 }

lib/SIL/Verifier/VerifierPrivate.h

@@ -41,7 +41,7 @@ class LoadBorrowImmutabilityAnalysis {
 private:
   bool isImmutableInScope(LoadBorrowInst *lbi,
                           ArrayRef<Operand *> endBorrowUses,
-                          AccessPath accessPath);
+                          AccessPathWithBase accessPathWithBase);
 };
 
 } // namespace silverifier

test/SIL/ownership-verifier/load_borrow_invalidation_test.sil

@@ -428,3 +428,29 @@ bb0(%0 : @owned $Builtin.NativeObject):
   %9999 = tuple()
   return %9999 : $()
 }
+
+class ComplexStruct {
+  var val : NonTrivialStruct
+}
+
+class Klass {
+}
+
+struct NonTrivialStruct {
+  var val : Klass
+}
+
+sil [ossa] @test_borrow : $@convention(thin) (@owned ComplexStruct) -> () {
+bb0(%0 : @owned $ComplexStruct):
+  %2 = copy_value %0 : $ComplexStruct
+  %4 = begin_borrow %2 : $ComplexStruct
+  %5 = ref_element_addr %4 : $ComplexStruct, #ComplexStruct.val
+  %6 = load_borrow %5 : $*NonTrivialStruct
+  destroy_value %0 : $ComplexStruct
+  end_borrow %6 : $NonTrivialStruct
+  end_borrow %4 : $ComplexStruct
+  destroy_value %2 : $ComplexStruct
+  %ret = tuple ()
+  return %ret : $()
+}
+