Merge pull request #64366 from gottesmm/pr-63fcd3307c126048533239ab77552bb5acdc728c

gottesmm · web-flow · commit b3bbaec7093b · 2023-03-14T17:12:11.000-07:00
[move-only] Treat mark_must_check assignable_but_not_consumable as ending initialized
diff --git a/lib/SILGen/SILGenLValue.cpp b/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);
     }
 
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp
@@ -449,34 +449,54 @@ static bool memInstMustConsume(Operand *memOper) {
 /// These are cases where we want to treat the end of the function as a liveness
 /// use to ensure that we reinitialize \p value before the end of the function
 /// if we consume \p value in the function body.
-static bool isInOutDefThatNeedsEndOfFunctionLiveness(SILValue value) {
-  if (auto *fArg = dyn_cast<SILFunctionArgument>(value)) {
-    switch (fArg->getArgumentConvention()) {
-    case SILArgumentConvention::Indirect_In:
-    case SILArgumentConvention::Indirect_Out:
-    case SILArgumentConvention::Indirect_In_Guaranteed:
-    case SILArgumentConvention::Direct_Guaranteed:
-    case SILArgumentConvention::Direct_Owned:
-    case SILArgumentConvention::Direct_Unowned:
-    case SILArgumentConvention::Pack_Guaranteed:
-    case SILArgumentConvention::Pack_Owned:
-    case SILArgumentConvention::Pack_Out:
-      return false;
-    case SILArgumentConvention::Indirect_Inout:
-    case SILArgumentConvention::Indirect_InoutAliasable:
-    case SILArgumentConvention::Pack_Inout:
-      LLVM_DEBUG(llvm::dbgs() << "Found inout arg: " << *fArg);
-      return true;
+static bool isInOutDefThatNeedsEndOfFunctionLiveness(MarkMustCheckInst *markedAddr) {
+  SILValue operand = markedAddr->getOperand();
+
+  // Check for inout types of arguments that are marked with consumable and
+  // assignable.
+  if (markedAddr->getCheckKind() ==
+      MarkMustCheckInst::CheckKind::ConsumableAndAssignable) {
+    if (auto *fArg = dyn_cast<SILFunctionArgument>(operand)) {
+      switch (fArg->getArgumentConvention()) {
+      case SILArgumentConvention::Indirect_In:
+      case SILArgumentConvention::Indirect_Out:
+      case SILArgumentConvention::Indirect_In_Guaranteed:
+      case SILArgumentConvention::Direct_Guaranteed:
+      case SILArgumentConvention::Direct_Owned:
+      case SILArgumentConvention::Direct_Unowned:
+      case SILArgumentConvention::Pack_Guaranteed:
+      case SILArgumentConvention::Pack_Owned:
+      case SILArgumentConvention::Pack_Out:
+        return false;
+      case SILArgumentConvention::Indirect_Inout:
+      case SILArgumentConvention::Indirect_InoutAliasable:
+      case SILArgumentConvention::Pack_Inout:
+        LLVM_DEBUG(llvm::dbgs() << "Found inout arg: " << *fArg);
+        return true;
+      }
     }
   }
 
-  if (auto *pbi = dyn_cast<ProjectBoxInst>(value)) {
-    if (auto *fArg = dyn_cast<SILFunctionArgument>(pbi->getOperand())) {
-      if (!fArg->isClosureCapture())
-        return false;
-      LLVM_DEBUG(llvm::dbgs() << "Found inout arg: " << *fArg);
-      return true;
+  // See if we have an assignable_but_not_consumable from a project_box +
+  // function_argument. In this case, the value must be live at the end of the
+  // use, similar to an inout parameter.
+  //
+  // TODO: Rather than using a terminator, we might be able to use the
+  // end_access of the access marker instead. That would slightly change the
+  // model and this is semantically ok today.
+  if (markedAddr->getCheckKind() ==
+      MarkMustCheckInst::CheckKind::AssignableButNotConsumable) {
+    if (auto *pbi = dyn_cast<ProjectBoxInst>(stripAccessMarkers(operand))) {
+      if (auto *fArg = dyn_cast<SILFunctionArgument>(pbi->getOperand())) {
+        if (!fArg->isClosureCapture())
+          return false;
+        LLVM_DEBUG(llvm::dbgs() << "Found inout arg: " << *fArg);
+        return true;
+      }
     }
+
+    if (isa<RefElementAddrInst>(stripAccessMarkers(operand)))
+      return true;
   }
 
   return false;
@@ -592,7 +612,7 @@ struct UseState {
   initializeLiveness(FieldSensitiveMultiDefPrunedLiveRange &prunedLiveness);
 
   void initializeInOutTermUsers() {
-    if (!isInOutDefThatNeedsEndOfFunctionLiveness(address->getOperand()))
+    if (!isInOutDefThatNeedsEndOfFunctionLiveness(address))
       return;
 
     SmallVector<SILBasicBlock *, 8> exitBlocks;
diff --git a/test/Interpreter/moveonly_escaping_capture_lifetimes.swift b/test/Interpreter/moveonly_escaping_capture_lifetimes.swift
@@ -0,0 +1,47 @@
+// RUN: %target-run-simple-swift(-Xfrontend -enable-experimental-move-only) | %FileCheck %s
+
+// REQUIRES: executable_test
+
+@_moveOnly
+struct MO {
+    var x: Int
+    deinit { print("dying \(x)") }
+}
+
+var f: () -> () = {}
+
+func foo(x: consuming MO) {
+    var counter = 42
+    f = {
+        x = MO(x: counter)
+        counter += 1
+    }
+}
+
+func main() {
+    let x = MO(x: 69)
+
+    // CHECK: a
+    print("a")
+    foo(x: x)
+    // CHECK-NEXT: b
+    print("b")
+    // CHECK-NEXT: dying 69
+    // CHECK-NEXT: c
+    f()
+    print("c")
+    // CHECK-NEXT: dying 42
+    // CHECK-NEXT: d
+    f()
+    print("d")
+    // CHECK-NEXT: dying 43
+    // CHECK-NEXT: e
+    f()
+    print("e")
+    // CHECK-NEXT: dying 44
+    // CHECK-NEXT: f
+    f = {}
+    print("f")
+}
+main()
+print("done")
diff --git a/test/SILGen/moveonly.swift b/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 //
 ///////////////////////
diff --git a/test/SILOptimizer/moveonly_addresschecker.sil b/test/SILOptimizer/moveonly_addresschecker.sil
@@ -27,6 +27,7 @@ public struct NonTrivialStruct {
 }
 
 sil @useNonTrivialStruct : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
+sil @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
 
 @_moveOnly
 public struct NonTrivialStructPair {
@@ -437,3 +438,58 @@ bb0(%0 : $*Klass):
   %27 = tuple ()
   return %27 : $()
 }
+
+// Make sure that we treat the move only access below like an inout parameter
+// that is live at the end of its lifetime, rather than like a var.
+//
+// CHECK: sil [ossa] @closure_assignable_but_not_consumable_treated_like_inout : $@convention(thin) (@guaranteed { var NonTrivialStruct }) -> () {
+// CHECK: bb0([[ARG:%.*]] : @closureCapture @guaranteed
+// CHECK-NEXT: project_box
+// CHECK-NEXT: // function_ref
+// CHECK-NEXT: function_ref
+// CHECK-NEXT: apply
+// CHECK-NEXT: begin_access
+// CHECK-NEXT: destroy_addr
+// CHECK-NEXT: store
+// CHECK-NEXT: end_access
+// CHECK-NEXT: tuple ()
+// CHECK-NEXT: return
+// CHECK-NEXT: } // end sil function 'closure_assignable_but_not_consumable_treated_like_inout'
+sil [ossa] @closure_assignable_but_not_consumable_treated_like_inout : $@convention(thin) (@guaranteed { var NonTrivialStruct }) -> () {
+bb0(%0 : @closureCapture @guaranteed ${ var NonTrivialStruct }):
+  %4 = project_box %0 : ${ var NonTrivialStruct }, 0
+  %9 = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
+  %10 = apply %9() : $@convention(thin) () -> @owned NonTrivialStruct
+  %11 = begin_access [modify] [dynamic] %4 : $*NonTrivialStruct
+  %12 = mark_must_check [assignable_but_not_consumable] %11 : $*NonTrivialStruct
+  store %10 to [assign] %12 : $*NonTrivialStruct
+  end_access %11 : $*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 : $()
+}
diff --git a/test/SILOptimizer/moveonly_coro_accessor.swift b/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
