[move-only] When adding implicit liveness uses for class field/global accesses, do not use the terminator, use the end access.

Previously, when doing this I could just use the terminator both in cases of
inout and for class field/global accesses... but after some recent changes to
field sensitive pruned liveness, this seems to no longer work. So just do the
right thing and use the field access.

The specific bug was that we would introduce a destroy_addr along one of the
paths where the value wasn't defined resulting in a dominance error.

I added a SIL test that shows this fixes the issue, a swift test, and also an
Interpreter test that validates the correctness.

rdar://111659649

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -385,17 +385,6 @@ static bool isInOutDefThatNeedsEndOfFunctionLiveness(MarkMustCheckInst *markedAd
     }
   }
 
-  // See if we have an assignable_but_not_consumable from a formal access.
-  // In this case, the value must be live at the end of the
-  // access, similar to an inout parameter.
-  if (markedAddr->getCheckKind() ==
-      MarkMustCheckInst::CheckKind::AssignableButNotConsumable) {
-    if (isa<BeginAccessInst>(operand)) {
-      return true;
-    }
-  }
-
-
   return false;
 }
 
@@ -513,7 +502,7 @@ struct UseState {
   /// terminator user and so that we can use the set to quickly identify later
   /// while emitting diagnostics that a liveness use is a terminator user and
   /// emit a specific diagnostic message.
-  SmallSetVector<SILInstruction *, 2> inoutTermUsers;
+  SmallSetVector<SILInstruction *, 2> implicitEndOfLifetimeLivenessUses;
 
   /// We add debug_values to liveness late after we diagnose, but before we
   /// hoist destroys to ensure that we do not hoist destroys out of access
@@ -565,11 +554,17 @@ struct UseState {
     setAffectedBits(inst, range, initInsts);
   }
 
-  /// Returns true if this is a terminator instruction that although it doesn't
-  /// use our inout argument directly is used by the pass to ensure that we
-  /// reinit said argument if we consumed it in the body of the function.
-  bool isInOutTermUser(SILInstruction *inst) const {
-    return inoutTermUsers.count(inst);
+  /// Returns true if this is an instruction that is used by the pass to ensure
+  /// that we reinit said argument if we consumed it in a region of code.
+  ///
+  /// Example:
+  ///
+  /// 1. In the case of an inout argument, this will contain the terminator
+  /// instruction.
+  /// 2. In the case of a ref_element_addr or a global, this will contain the
+  /// end_access.
+  bool isImplicitEndOfLifetimeLivenessUses(SILInstruction *inst) const {
+    return implicitEndOfLifetimeLivenessUses.count(inst);
   }
 
   /// Returns true if the given instruction is within the same block as a reinit
@@ -609,7 +604,7 @@ struct UseState {
     initInsts.clear();
     reinitInsts.clear();
     dropDeinitInsts.clear();
-    inoutTermUsers.clear();
+    implicitEndOfLifetimeLivenessUses.clear();
     debugValue = nullptr;
   }
 
@@ -647,8 +642,8 @@ struct UseState {
     for (auto *inst : dropDeinitInsts) {
       llvm::dbgs() << *inst;
     }
-    llvm::dbgs() << "InOut Term Users:\n";
-    for (auto *inst : inoutTermUsers) {
+    llvm::dbgs() << "Implicit End Of Lifetime Liveness Users:\n";
+    for (auto *inst : implicitEndOfLifetimeLivenessUses) {
       llvm::dbgs() << *inst;
     }
     llvm::dbgs() << "Debug Value User:\n";
@@ -680,16 +675,28 @@ struct UseState {
   void
   initializeLiveness(FieldSensitiveMultiDefPrunedLiveRange &prunedLiveness);
 
-  void initializeInOutTermUsers() {
-    if (!isInOutDefThatNeedsEndOfFunctionLiveness(address))
+  void initializeImplicitEndOfLifetimeLivenessUses() {
+    if (isInOutDefThatNeedsEndOfFunctionLiveness(address)) {
+      SmallVector<SILBasicBlock *, 8> exitBlocks;
+      address->getFunction()->findExitingBlocks(exitBlocks);
+      for (auto *block : exitBlocks) {
+        LLVM_DEBUG(llvm::dbgs() << "    Adding term as liveness user: "
+                                << *block->getTerminator());
+        implicitEndOfLifetimeLivenessUses.insert(block->getTerminator());
+      }
       return;
+    }
 
-    SmallVector<SILBasicBlock *, 8> exitBlocks;
-    address->getFunction()->findExitingBlocks(exitBlocks);
-    for (auto *block : exitBlocks) {
-      LLVM_DEBUG(llvm::dbgs() << "    Adding term as liveness user: "
-                              << *block->getTerminator());
-      inoutTermUsers.insert(block->getTerminator());
+    if (address->getCheckKind() ==
+        MarkMustCheckInst::CheckKind::AssignableButNotConsumable) {
+      if (auto *bai = dyn_cast<BeginAccessInst>(address->getOperand())) {
+        for (auto *eai : bai->getEndAccesses()) {
+          LLVM_DEBUG(llvm::dbgs() << "    Adding end_access as implicit end of "
+                                     "lifetime liveness user: "
+                                  << *eai);
+          implicitEndOfLifetimeLivenessUses.insert(eai);
+        }
+      }
     }
   }
 
@@ -749,7 +756,7 @@ struct UseState {
     // An "inout terminator use" is an implicit liveness use of the entire
     // value. This is because we need to ensure that our inout value is
     // reinitialized along exit paths.
-    if (inoutTermUsers.count(inst))
+    if (implicitEndOfLifetimeLivenessUses.count(inst))
       return true;
 
     return false;
@@ -1064,13 +1071,11 @@ void UseState::initializeLiveness(
                liveness.print(llvm::dbgs()));
   }
 
-  // Finally, if we have an inout argument, add a liveness use of the entire
-  // value on terminators in blocks that are exits from the function. This
-  // ensures that along all paths, if our inout is not reinitialized before we
-  // exit the function, we will get an error. We also stash these users into
-  // inoutTermUser so we can quickly recognize them later and emit a better
-  // error msg.
-  for (auto *inst : inoutTermUsers) {
+  // Finally, if we have an inout argument or an access scope associated with a
+  // ref_element_addr or global_addr, add a liveness use of the entire value on
+  // the implicit end lifetime instruction. For inout this is terminators for
+  // ref_element_addr, global_addr it is the end_access instruction.
+  for (auto *inst : implicitEndOfLifetimeLivenessUses) {
     liveness.updateForUse(inst, TypeTreeLeafTypeRange(address),
                           false /*lifetime ending*/);
     LLVM_DEBUG(llvm::dbgs() << "Added liveness for inoutTermUser: " << *inst;
@@ -2229,7 +2234,7 @@ bool GlobalLivenessChecker::testInstVectorLiveness(
         if (addressUseState.isLivenessUse(&*ii, errorSpan)) {
           diagnosticEmitter.emitAddressDiagnostic(
               addressUseState.address, &*ii, errorUser, false /*is consuming*/,
-              addressUseState.isInOutTermUser(&*ii));
+              addressUseState.isImplicitEndOfLifetimeLivenessUses(&*ii));
           foundSingleBlockError = true;
           emittedDiagnostic = true;
           break;
@@ -2249,8 +2254,9 @@ bool GlobalLivenessChecker::testInstVectorLiveness(
                   &*ii, FieldSensitivePrunedLiveness::NonLifetimeEndingUse,
                   errorSpan)) {
             diagnosticEmitter.emitAddressDiagnostic(
-                addressUseState.address, &*ii, errorUser, false /*is consuming*/,
-                addressUseState.isInOutTermUser(&*ii));
+                addressUseState.address, &*ii, errorUser,
+                false /*is consuming*/,
+                addressUseState.isImplicitEndOfLifetimeLivenessUses(&*ii));
             foundSingleBlockError = true;
             emittedDiagnostic = true;
             break;
@@ -2340,7 +2346,8 @@ bool GlobalLivenessChecker::testInstVectorLiveness(
               diagnosticEmitter.emitAddressDiagnostic(
                   addressUseState.address, &blockInst, errorUser,
                   false /*is consuming*/,
-                  addressUseState.isInOutTermUser(&blockInst));
+                  addressUseState.isImplicitEndOfLifetimeLivenessUses(
+                      &blockInst));
               foundSingleBlockError = true;
               emittedDiagnostic = true;
               break;
@@ -2567,6 +2574,18 @@ void MoveOnlyAddressCheckerPImpl::insertDestroysOnBoundary(
           continue;
         }
 
+        // If we have an implicit end of lifetime use, we do not insert a
+        // destroy_addr. Instead, we insert an undef debug value after the
+        // use. This occurs if we have an end_access associated with a
+        // global_addr or a ref_element_addr field access.
+        if (addressUseState.isImplicitEndOfLifetimeLivenessUses(inst)) {
+          LLVM_DEBUG(
+              llvm::dbgs()
+              << "    Use was an implicit end of lifetime liveness use!\n");
+          insertUndefDebugValue(inst->getNextInstruction());
+          continue;
+        }
+
         auto *insertPt = inst->getNextInstruction();
         insertDestroyBeforeInstruction(addressUseState, insertPt,
                                        liveness.getRootValue(), bits, consumes);
@@ -3207,7 +3226,7 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
   // DISCUSSION: For non address only types, this is not an issue since we
   // eagerly load
 
-  addressUseState.initializeInOutTermUsers();
+  addressUseState.initializeImplicitEndOfLifetimeLivenessUses();
 
   //===---
   // Liveness Checking

test/Interpreter/moveonly_linkedlist.swift

@@ -0,0 +1,136 @@
+// RUN: %target-run-simple-swift(-Xfrontend -sil-verify-all)
+// RUN: %target-run-simple-swift(-O -Xfrontend -sil-verify-all)
+
+// REQUIRES: executable_test
+
+/// A class that we use as a box to store the memory for one of our linked list
+/// nodes. It is on purpose fileprivate since it is an implementation detail of
+/// \p NodeBox.
+fileprivate final class _Box<T> {
+  var value: _Node<T>
+
+  init(_ value: consuming _Node<T>) { self.value = value }
+}
+
+struct _Node<T> : ~Copyable {
+  var value: T
+  var _next: ListEntry<T> = ListEntry<T>()
+
+  init(_ newValue: T) {
+    value = newValue
+  }
+}
+
+/// A noncopyable box that contains the memory for a linked list node. Can be
+/// embedded within other noncopyable data structures to point at a Node data
+/// structure.
+///
+/// Internally uses a class as the actual box.
+struct ListEntry<T> : ~Copyable {
+  private var innerBox: _Box<T>?
+
+  init() { innerBox = nil }
+  init(initialValue value: consuming T) {
+    innerBox = _Box<T>(_Node(value))
+  }
+
+  mutating func push(value newValue: consuming T) {
+    if innerBox == nil {
+      // If we do not already have a head, just take on this value and return.
+      innerBox = _Box<T>(_Node(newValue))
+      return
+    }
+
+    // Otherwise, we need to create a new node and fix things up.
+    var nodeEntry = ListEntry<T>(initialValue: newValue)
+    nodeEntry.next = self
+    self = nodeEntry
+  }
+
+  mutating func pop() -> T? {
+    guard let innerBox = innerBox else {
+      return nil
+    }
+
+    let result = innerBox.value.value
+    func fixNext(_ lhs: inout ListEntry<T>, _ rhs: inout ListEntry<T>) {
+      lhs = rhs
+      rhs = ListEntry<T>()
+    }
+    fixNext(&self, &innerBox.value._next)
+    return result
+  }
+
+  var hasNext: Bool {
+    return innerBox != nil
+  }
+  var next: ListEntry<T> {
+    _modify {
+      yield &innerBox!.value._next
+    }
+    _read {
+      yield innerBox!.value._next
+    }
+  }
+
+  var value: T? {
+    return innerBox?.value.value
+  }
+}
+
+let target = "ogyfbssvlh"
+let strings = [
+  "nulbhqylps",
+  "hpdovhuybl",
+  "bjjvpakqbm",
+  "rqyozjzkyz",
+  "qpzghmdcag",
+  "lqefxvulvn",
+  "wtokfqarxm",
+  "acdcrzxpdg",
+  "bxgfacpjic",
+  "acblrvoego",
+  "msevhriohn",
+  "bamfcnbqvx",
+  "wimkkqhryd",
+  "dounctqkiw",
+  "zxmyxcabhq",
+  "ljerkuhlgy",
+  "cettadahue",
+  "cuummvmwly",
+  "kdebludzsh",
+  "ogyfbssvlh",
+  "lrowrxwufj",
+  "rftifkqggr",
+  "ktjeeeobca",
+  "xqlbnswmjr",
+  "zpuxfbtmip",
+  "rljcxrvdgh",
+  "twkgardobr",
+  "zrogczpzem",
+  "bkuzjugksg",
+  "eqanimdywo"
+]
+
+func buildList() -> ListEntry<String> {
+  var head = ListEntry<String>()
+
+  for i in strings {
+    head.push(value: i)
+  }
+
+  return head
+}
+
+func main() {
+  var head = buildList()
+  var count = 0
+
+  var strCount = strings.count
+  while let x = head.pop() {
+      assert(x == strings[strCount - count - 1])
+      count = count + 1
+  }
+}
+
+main()

test/SILOptimizer/moveonly_addresschecker.sil

@@ -732,3 +732,39 @@ bb0(%0 : @owned $FixedSizeQueue):
   dealloc_stack %1 : $*FixedSizeQueue
   return %11 : $Builtin.Int32
 }
+
+// Make sure that we set the end_access as the implicit end lifetime use instead
+// of the terminator.
+// CHECK-LABEL: sil [ossa] @assignableButNotConsumableEndAccessImplicitLifetimeTest : $@convention(thin) (@guaranteed ClassContainingMoveOnly, @owned NonTrivialStruct2) -> () {
+// CHECK: bb0([[ARG0:%.*]] : @guaranteed $ClassContainingMoveOnly, [[ARG1:%.*]] : @owned
+// CHECK: bb2:
+// CHECK-NEXT:   [[ADDR:%.*]] = ref_element_addr [[ARG0]]
+// CHECK-NEXT:   [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[ADDR]]
+// CHECK-NEXT:   [[GEP1:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK-NEXT:   [[GEP2:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK-NEXT:   destroy_addr [[GEP2]]
+// CHECK-NEXT:   store [[ARG1]] to [init] [[GEP1]]
+// CHECK-NEXT:   end_access [[ACCESS]]
+// CHECK-NEXT:   br bb3
+// CHECK: } // end sil function 'assignableButNotConsumableEndAccessImplicitLifetimeTest'
+sil [ossa] @assignableButNotConsumableEndAccessImplicitLifetimeTest : $@convention(thin) (@guaranteed ClassContainingMoveOnly, @owned NonTrivialStruct2) -> () {
+bb0(%0 : @guaranteed $ClassContainingMoveOnly, %1 : @owned $NonTrivialStruct2):
+  cond_br undef, bb1, bb2
+
+bb1:
+  destroy_value %1 : $NonTrivialStruct2
+  br bb3
+
+bb2:
+  %2 = ref_element_addr %0 : $ClassContainingMoveOnly, #ClassContainingMoveOnly.value
+  %3 = begin_access [modify] [dynamic] %2 : $*NonTrivialStruct
+  %4 = mark_must_check [assignable_but_not_consumable] %3 : $*NonTrivialStruct
+  %4a = struct_element_addr %4 : $*NonTrivialStruct, #NonTrivialStruct.nonTrivialStruct2
+  store %1 to [assign] %4a : $*NonTrivialStruct2
+  end_access %3 : $*NonTrivialStruct
+  br bb3
+
+bb3:
+  %9999 = tuple ()
+  return %9999 : $()
+}

test/SILOptimizer/moveonly_addresschecker_diagnostics.swift

@@ -4551,3 +4551,14 @@ public class NonFinalCopyableKlassWithMoveOnlyField {
     var moveOnlyVarProt = AddressOnlyProtocol()
     let moveOnlyLetProt = AddressOnlyProtocol()
 }
+
+//////////////////////
+// MARK: Misc Tests //
+//////////////////////
+
+// For misc tests associated with specific radars.
+func assignableButNotConsumableEndAccessImplicitLifetimeTest(_ x: CopyableKlassWithMoveOnlyField) {
+    if boolValue {
+        x.moveOnlyVarStruct.nonTrivialStruct2 = NonTrivialStruct2()
+    }
+}