[move-only] Make sure to treat ref_element_addr mutable address accesses similar to inout.

I also slightly changed the codegen around where we insert the mark_must_check.
Specifically, before we would emit the mark_must_check directly on the
ref_element_addr and then insert the access. This had the unfortunate effect
that we would hoist any destroy_addr that were actually needed out of the access
scope. Rather than do that, I now insert the mark_must_check on the access
itself. This results in the destroy_addr being within the scope (like the
mark_must_check itself).

rdar://105910066

Author: gottesmm

lib/SILGen/SILGenLValue.cpp

@@ -790,17 +790,6 @@ namespace {
         SGF.B.createRefElementAddr(loc, base.getUnmanagedValue(),
                                    Field, SubstFieldType);
 
-      // If we have a move only type...
-      if (result->getType().isMoveOnly()) {
-        auto checkKind =
-            MarkMustCheckInst::CheckKind::AssignableButNotConsumable;
-        if (isReadAccess(getAccessKind())) {
-          // Add a mark_must_check [no_consume_or_assign].
-          checkKind = MarkMustCheckInst::CheckKind::NoConsumeOrAssign;
-        }
-        result = SGF.B.createMarkMustCheckInst(loc, result, checkKind);
-      }
-
       // Avoid emitting access markers completely for non-accesses or immutable
       // declarations. Access marker verification is aware of these cases.
       if (!IsNonAccessing && !Field->isLet()) {
@@ -811,6 +800,20 @@ namespace {
         }
       }
 
+      // If we have a move only type, add a marker.
+      //
+      // NOTE: We purposely do this on the access itself to ensure that when we
+      // hoist destroy_addr, they stay within the access scope.
+      if (result->getType().isMoveOnly()) {
+        auto checkKind =
+            MarkMustCheckInst::CheckKind::AssignableButNotConsumable;
+        if (isReadAccess(getAccessKind())) {
+          // Add a mark_must_check [no_consume_or_assign].
+          checkKind = MarkMustCheckInst::CheckKind::NoConsumeOrAssign;
+        }
+        result = SGF.B.createMarkMustCheckInst(loc, result, checkKind);
+      }
+
       return ManagedValue::forLValue(result);
     }
 

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -383,6 +383,9 @@ static bool isInOutDefThatNeedsEndOfFunctionLiveness(MarkMustCheckInst *markedAd
         return true;
       }
     }
+
+    if (isa<RefElementAddrInst>(stripAccessMarkers(operand)))
+      return true;
   }
 
   return false;

test/SILGen/moveonly.swift

@@ -537,9 +537,9 @@ func moveOnlyStructCopyableStructCopyableStructCopyableKlassNonConsumingUse() {
 // CHECK:   end_access [[ACCESS]]
 // CHECK:   [[BORROWED_COPYABLE_KLASS:%.*]] = begin_borrow [[COPYABLE_KLASS]]
 // CHECK:   [[FIELD:%.*]] = ref_element_addr [[BORROWED_COPYABLE_KLASS]]
-// CHECK:   [[FIELD_MARK:%.*]] = mark_must_check [no_consume_or_assign] [[FIELD]]
-// CHECK:   [[ACCESS:%.*]] = begin_access [read] [dynamic] [[FIELD_MARK]]
-// CHECK:   [[BORROWED_MOVEONLY_KLASS:%.*]] = load_borrow [[ACCESS]]
+// CHECK:   [[ACCESS:%.*]] = begin_access [read] [dynamic] [[FIELD]]
+// CHECK:   [[ACCESS_MARK:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
+// CHECK:   [[BORROWED_MOVEONLY_KLASS:%.*]] = load_borrow [[ACCESS_MARK]]
 // CHECK:   [[FN:%.*]] = function_ref @$s8moveonly9borrowValyyAA2FDVhF :
 // CHECK:   apply [[FN]]([[BORROWED_MOVEONLY_KLASS]])
 // CHECK:   end_borrow [[BORROWED_MOVEONLY_KLASS]]
@@ -551,6 +551,22 @@ func moveOnlyStructCopyableStructCopyableStructCopyableKlassMoveOnlyKlassNonCons
     borrowVal(k.nonTrivialCopyableStruct.nonTrivialCopyableStruct2.copyableKlass.fd)
 }
 
+//////////////////////
+// Assignment Tests //
+//////////////////////
+
+// CHECK-LABEL: sil hidden [ossa] @$s8moveonly19assignCopyableKlassyyAA0cD0CF : $@convention(thin) (@guaranteed CopyableKlass) -> () {
+// CHECK: bb0([[ARG:%.*]] : @guaranteed
+// CHECK:   [[REF:%.*]] = ref_element_addr [[ARG]]
+// CHECK:   [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[REF]]
+// CHECK:   [[MARKED_ACCESS:%.*]] = mark_must_check [assignable_but_not_consumable] [[ACCESS]]
+// CHECK:   assign {{%.*}} to [[MARKED_ACCESS]]
+// CHECK:   end_access [[ACCESS]]
+// CHECK: } // end sil function '$s8moveonly19assignCopyableKlassyyAA0cD0CF'
+func assignCopyableKlass(_ x: CopyableKlass) {
+    x.fd = FD()
+}
+
 ///////////////////////
 // Enum Switch Tests //
 ///////////////////////

test/SILOptimizer/moveonly_addresschecker.sil

@@ -467,3 +467,29 @@ bb0(%0 : @closureCapture @guaranteed ${ var NonTrivialStruct }):
   %24 = tuple ()
   return %24 : $()
 }
+
+// CHECK: sil hidden [ossa] @ref_element_addr_treated_like_inout : $@convention(method) (@guaranteed ClassContainingMoveOnly) -> () {
+// CHECK: bb0(
+// CHECK-NEXT: // function_ref
+// CHECK-NEXT: function_ref
+// CHECK-NEXT: apply
+// CHECK-NEXT: ref_element_addr
+// CHECK-NEXT: begin_access
+// CHECK-NEXT: destroy_addr
+// CHECK-NEXT: store
+// CHECK-NEXT: end_access
+// CHECK-NEXT: tuple ()
+// CHECK-NEXT: return
+// CHECK: } // end sil function 'ref_element_addr_treated_like_inout'
+sil hidden [ossa] @ref_element_addr_treated_like_inout : $@convention(method) (@guaranteed ClassContainingMoveOnly) -> () {
+bb0(%0 : @guaranteed $ClassContainingMoveOnly):
+  %9 = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
+  %10 = apply %9() : $@convention(thin) () -> @owned NonTrivialStruct
+  %5 = ref_element_addr %0 : $ClassContainingMoveOnly, #ClassContainingMoveOnly.value
+  %6 = begin_access [modify] [dynamic] %5 : $*NonTrivialStruct
+  %7 = mark_must_check [assignable_but_not_consumable] %6 : $*NonTrivialStruct
+  store %10 to [assign] %7 : $*NonTrivialStruct
+  end_access %6 : $*NonTrivialStruct
+  %11 = tuple ()
+  return %11 : $()
+}

test/SILOptimizer/moveonly_coro_accessor.swift

@@ -43,12 +43,10 @@ public class ListOfFiles {
 // CHECK:    [[NEW_VAL_RELOADED:%.*]] = load [[NEW_VAL_STACK]] : $*File
 // >> destroy the element currently in the field
 // CHECK:    [[FIELD:%.*]] = ref_element_addr [[SELF]] : $ListOfFiles, #ListOfFiles.file
-// CHECK:    destroy_addr [[FIELD]] : $*File
 // >> write the new value in its place
 // CHECK:    [[FIELD_ACCESS:%.*]] = begin_access [modify] [dynamic] [[FIELD]] : $*File
+// CHECK:    destroy_addr [[FIELD_ACCESS]] : $*File
 // CHECK:    store [[NEW_VAL_RELOADED]] to [[FIELD_ACCESS]] : $*File
-// >> FIXME: we should not be destroying the field here  rdar://105910066
-// CHECK:    destroy_addr [[FIELD]] : $*File
 //
 // CHECK:    end_access [[FIELD_ACCESS]] : $*File
 // CHECK-NOT: begin_access
@@ -65,15 +63,13 @@ public class ListOfFiles {
 // CHECK:    yield [[ACCESS]] : $*File, resume bb1, unwind bb2
 //
 // CHECK:    bb1:
-// >> FIXME: we should not be destroying the field here  rdar://105910066
-// CHECK:    destroy_addr [[FIELD]] : $*File
 //
 // CHECK:    end_access [[ACCESS]] : $*File
 // CHECK-NOT: begin_access
+// CHECK-NOT:    destroy_addr [[FIELD]] : $*File
 //
 // CHECK:    bb2:
-// >> FIXME: we should not be destroying the field here  rdar://105910066
-// CHECK:    destroy_addr [[FIELD]] : $*File
+// CHECK-NOT:    destroy_addr [[FIELD]] : $*File
 //
 // CHECK:    end_access [[ACCESS]] : $*File
 // CHECK-NOT: begin_access