[move-only] Remove misguided handling of inout.

I think this was just an incorrect fix. The actual issue is that the memory
lifetime verifier views debug_value as requiring a live address... but at the
same time, we do not want to emit diagnostics for a debug_value... we want to do
it for other things. So my solution is to add debug_value to the final liveness
computation, but not use it earlier when we use said liveness to compute
diagnostics.

rdar://106442224

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -410,6 +410,11 @@ struct UseState {
   llvm::SmallMapVector<SILInstruction *, TypeTreeLeafTypeRange, 4> reinitInsts;
   SmallSetVector<SILInstruction *, 2> inoutTermUsers;
 
+  /// 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
+  /// scopes.
+  DebugValueInst *debugValue = nullptr;
+
   SILFunction *getFunction() const { return address->getFunction(); }
 
   /// Returns true if this is a terminator instruction that although it doesn't
@@ -429,6 +434,7 @@ struct UseState {
     initInsts.clear();
     reinitInsts.clear();
     inoutTermUsers.clear();
+    debugValue = nullptr;
   }
 
   void dump() {
@@ -465,6 +471,10 @@ struct UseState {
     for (auto *inst : inoutTermUsers) {
       llvm::dbgs() << *inst;
     }
+    llvm::dbgs() << "Debug Value User:\n";
+    if (debugValue) {
+      llvm::dbgs() << *debugValue;
+    }
   }
 
   void
@@ -1198,10 +1208,6 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
     return true;
   }
 
-  // We don't care about debug instructions.
-  if (op->getUser()->isDebugInstruction())
-    return true;
-
   // For convenience, grab the user of op.
   auto *user = op->getUser();
 
@@ -1261,6 +1267,19 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
   if (isa<EndAccessInst>(user))
     return true;
 
+  if (auto *di = dyn_cast<DebugValueInst>(user)) {
+    // Make sure that our debug_value is always on our root value. If not, we
+    // have something we don't understand and should bail. This ensures we can
+    // always hoist the debug_value to our mark_must_check. This ensures that by
+    // marking debug_value later as requiring liveness, we do not change our
+    // liveness calculation since values are always live at the mark_must_check.
+    if (di->getOperand() != getRootAddress())
+      return false;
+
+    useState.debugValue = di;
+    return true;
+  }
+
   // At this point, we have handled all of the non-loadTakeOrCopy/consuming
   // uses.
   if (auto *copyAddr = dyn_cast<CopyAddrInst>(user)) {
@@ -1943,21 +1962,6 @@ void MoveOnlyAddressCheckerPImpl::insertDestroysOnBoundary(
                                      liveness.getRootValue(), defPair.second,
                                      consumes);
     } else {
-      // If our dead def is a mark_must_check and we are processing an inout
-      // argument, do not insert a destroy_addr. We are cheating a little bit by
-      // modeling the initial value as a mark_must_check... so we need to
-      // compensate for our cheating by not inserting the destroy_addr here
-      // since we would be destroying the inout argument before we use it.
-      if (auto *markMustCheckInst =
-              dyn_cast<MarkMustCheckInst>(defPair.first)) {
-        if (auto *arg = dyn_cast<SILFunctionArgument>(
-                markMustCheckInst->getOperand())) {
-          if (arg->getArgumentConvention().isInoutConvention()) {
-            continue;
-          }
-        }
-      }
-
       auto *inst = cast<SILInstruction>(defPair.first);
       auto *insertPt = inst->getNextInstruction();
       assert(insertPt && "def instruction was a terminator");
@@ -2144,9 +2148,28 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
     return true;
   }
 
+  //===
+  // Final Transformation
+  //
+
   // Ok, we now know that we fit our model since we did not emit errors and thus
   // can begin the transformation.
   SWIFT_DEFER { consumes.clear(); };
+
+  // First add any debug_values that we saw as liveness uses. This is important
+  // since the debugger wants to see live values when we define a debug_value,
+  // but we do not want to use them earlier when emitting diagnostic errors.
+  if (auto *di = addressUseState.debugValue) {
+    // Move the debug_value to right before the markedAddress to ensure that we
+    // do not actually change our liveness computation.
+    //
+    // NOTE: The author is not sure if this can ever happen with SILGen output,
+    // but this is being put just to be safe.
+    di->moveAfter(markedAddress);
+    liveness.updateForUse(di, TypeTreeLeafTypeRange(markedAddress),
+                          false /*lifetime ending*/);
+  }
+
   FieldSensitivePrunedLivenessBoundary boundary(liveness.getNumSubElements());
   liveness.computeBoundary(boundary);
   insertDestroysOnBoundary(markedAddress, liveness, boundary);

test/SILOptimizer/moveonly_addresschecker.sil

@@ -339,7 +339,14 @@ bb0(%0 : @owned $Klass, %1 : @owned $Klass):
 }
 
 // CHECK-LABEL: sil [ossa] @myBufferViewSetter : $@convention(method) <T> (UnsafeBufferPointer<T>, @inout MyBufferView<T>) -> () {
-// CHECK-NOT: destroy_addr
+// CHECK: bb0([[ARG0:%.*]] : ${{.*}}, [[ARG1:%.*]] :
+// CHECK:   debug_value [[ARG0]]
+// CHECK:   debug_value [[ARG1]]
+// CHECK:   destroy_addr [[ARG1]]
+// CHECK:   [[ACCESS:%.*]] = begin_access [modify] [static] [[ARG1]]
+// CHECK:   [[GEP:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK:   store [[ARG0]] to [trivial] [[GEP]]
+// CHECK:   end_access [[ACCESS]]
 // CHECK: } // end sil function 'myBufferViewSetter'
 sil [ossa] @myBufferViewSetter : $@convention(method) <T> (UnsafeBufferPointer<T>, @inout MyBufferView<T>) -> () {
 bb0(%0 : $UnsafeBufferPointer<T>, %1 : $*MyBufferView<T>):
@@ -408,3 +415,25 @@ bb0:
   %15 = tuple ()
   return %15 : $()
 }
+
+// CHECK-LABEL: sil [ossa] @classSimpleChainArgTestHoistDebugValue : $@convention(thin) (@inout Klass) -> () {
+// CHECK: bb0([[ARG:%.*]] :
+// CHECK-NEXT: debug_value [[ARG]]
+sil [ossa] @classSimpleChainArgTestHoistDebugValue : $@convention(thin) (@inout Klass) -> () {
+bb0(%0 : $*Klass):
+  %1 = mark_must_check [consumable_and_assignable] %0 : $*Klass
+  %3 = alloc_stack [lexical] $Klass, var, name "y2"
+  %4 = mark_must_check [consumable_and_assignable] %3 : $*Klass
+  %5 = begin_access [read] [static] %1 : $*Klass
+  copy_addr %5 to [init] %4 : $*Klass
+  end_access %5 : $*Klass
+  // If we do not hoist this to %1, we should crash. Make sure we dont!
+  debug_value %1 : $*Klass, var, name "x2", argno 1, expr op_deref
+  destroy_addr %4 : $*Klass
+  dealloc_stack %3 : $*Klass
+  %f = function_ref @get_klass : $@convention(thin) () -> @owned Klass
+  %newArg = apply %f() : $@convention(thin) () -> @owned Klass
+  store %newArg to [init] %1 : $*Klass
+  %27 = tuple ()
+  return %27 : $()
+}

test/SILOptimizer/moveonly_addresschecker_diagnostics.sil

@@ -279,3 +279,40 @@ bb0(%0 : @owned $NonTrivialStruct):
   %11 = tuple ()
   return %11 : $()
 }
+
+
+sil [ossa] @classSimpleChainArgTest : $@convention(thin) (@inout Klass) -> () {
+bb0(%0 : $*Klass):
+  %1 = mark_must_check [consumable_and_assignable] %0 : $*Klass
+  // expected-error @-1 {{'x2' consumed more than once}}
+  // expected-error @-2 {{'x2' consumed but not reinitialized before end of function}}
+  debug_value %1 : $*Klass, var, name "x2", argno 1, expr op_deref
+  %3 = alloc_stack [lexical] $Klass, var, name "y2"
+  %4 = mark_must_check [consumable_and_assignable] %3 : $*Klass
+  %5 = begin_access [read] [static] %1 : $*Klass
+  copy_addr %5 to [init] %4 : $*Klass // expected-note {{consuming use here}}
+  end_access %5 : $*Klass
+  %8 = begin_access [read] [static] %1 : $*Klass
+  %9 = load [copy] %8 : $*Klass
+  // expected-note @-1 {{consuming use here}}
+  // expected-note @-2 {{consuming use here}}
+  end_access %8 : $*Klass
+  %11 = begin_access [modify] [static] %4 : $*Klass
+  store %9 to [assign] %11 : $*Klass
+  end_access %11 : $*Klass
+  %14 = alloc_stack [lexical] $Klass, let, name "k2"
+  %15 = mark_must_check [consumable_and_assignable] %14 : $*Klass
+  %16 = begin_access [read] [static] %4 : $*Klass
+  copy_addr %16 to [init] %15 : $*Klass
+  end_access %16 : $*Klass
+  %19 = load_borrow %15 : $*Klass
+  %20 = function_ref @nonConsumingUseKlass : $@convention(thin) (@guaranteed Klass) -> ()
+  %21 = apply %20(%19) : $@convention(thin) (@guaranteed Klass) -> ()
+  end_borrow %19 : $Klass
+  destroy_addr %15 : $*Klass
+  dealloc_stack %14 : $*Klass
+  destroy_addr %4 : $*Klass
+  dealloc_stack %3 : $*Klass
+  %27 = tuple ()
+  return %27 : $()
+}