[move-only] Teach the move checker how to handle global_addr.

gottesmm · gottesmm · commit c832b41b7bdd · 2023-02-12T17:39:27.000-08:00
As part of this I also had to change how we emit global_addr in
SILGenLValue. Specifically, only for noncopyable types, we no longer emit a
single global_addr at the beginning of the function (in a sense auto-CSEing) and
instead always emit a new global_addr for each access. The reason why we do this
is that otherwise, access base visitor will consider all accesses to the global
to be for the same single access. In contrast, by always emitting the
global_addr each time, we provide a new base for each access allowing us to emit
the diagnostics that we want to.

rdar://102794400
diff --git a/lib/SILGen/SILGenGlobalVariable.cpp b/lib/SILGen/SILGenGlobalVariable.cpp
@@ -100,14 +100,22 @@ SILGenFunction::emitGlobalVariableRef(SILLocation loc, VarDecl *var,
     return ManagedValue::forLValue(addr);
   }
 
-  // Global variables can be accessed directly with global_addr.  Emit this
-  // instruction into the prolog of the function so we can memoize/CSE it in
-  // VarLocs.
+  // Global variables can be accessed directly with global_addr. If we have a
+  // noncopyable type, just emit the global_addr so each individual access has
+  // its own base projection. This is important so that the checker can
+  // distinguish in between different accesses to the same global.
+  auto *silG = SGM.getSILGlobalVariable(var, NotForDefinition);
+  if (silG->getLoweredType().isMoveOnly()) {
+    SILValue addr = B.createGlobalAddr(var, silG);
+    return ManagedValue::forLValue(addr);
+  }
+
+  // If we have a copyable type, emit this instruction into the prolog of the
+  // function so we can memoize/CSE it via the VarLocs map.
   auto *entryBB = &*getFunction().begin();
   SILGenBuilder prologueB(*this, entryBB, entryBB->begin());
   prologueB.setTrackingList(B.getTrackingList());
 
-  auto *silG = SGM.getSILGlobalVariable(var, NotForDefinition);
   SILValue addr = prologueB.createGlobalAddr(var, silG);
 
   VarLocs[var] = SILGenFunction::VarLoc::get(addr);
diff --git a/lib/SILGen/SILGenLValue.cpp b/lib/SILGen/SILGenLValue.cpp
@@ -2988,6 +2988,14 @@ void LValue::addNonMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
       // The only other case that should get here is a global variable.
       if (!address) {
         address = SGF.emitGlobalVariableRef(Loc, Storage, ActorIso);
+        if (address.getType().isMoveOnly()) {
+            auto checkKind =
+                MarkMustCheckInst::CheckKind::AssignableButNotConsumable;
+            if (isReadAccess(AccessKind)) {
+              checkKind = MarkMustCheckInst::CheckKind::NoConsumeOrAssign;
+            }
+            address = SGF.B.createMarkMustCheckInst(Loc, address, checkKind);
+        }
       } else {
         assert((!ActorIso || Storage->isTopLevelGlobal()) &&
                "local var should not be actor isolated!");
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp
@@ -615,6 +615,15 @@ void UseState::initializeLiveness(
     liveness.initializeDef(address, liveness.getTopLevelSpan());
   }
 
+  // Check if our address is from a global_addr. In such a case, we treat the
+  // mark_must_check as the initialization.
+  if (auto *globalAddr = dyn_cast<GlobalAddrInst>(address->getOperand())) {
+    LLVM_DEBUG(llvm::dbgs() << "Found global_addr use... "
+                               "adding mark_must_check as init!\n");
+    initInsts.insert({address, liveness.getTopLevelSpan()});
+    liveness.initializeDef(address, liveness.getTopLevelSpan());
+  }
+
   // Now that we have finished initialization of defs, change our multi-maps
   // from their array form to their map form.
   liveness.finishedInitializationOfDefs();
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp
@@ -59,11 +59,14 @@ static void diagnose(ASTContext &context, SILInstruction *inst, Diag<T...> diag,
   context.Diags.diagnose(loc.getSourceLoc(), diag, std::forward<U>(args)...);
 }
 
-static void getVariableNameForValue(MarkMustCheckInst *mmci,
+/// Helper function that actually implements getVariableNameForValue. Do not
+/// call it directly! Call the unary variants instead.
+static void getVariableNameForValue(SILValue value2,
+                                    SILValue searchValue,
                                     SmallString<64> &resultingString) {
   // Before we do anything, lets see if we have an exact debug_value on our
   // mmci. In such a case, we can end early and are done.
-  if (auto *use = getSingleDebugUse(mmci)) {
+  if (auto *use = getSingleDebugUse(value2)) {
     if (auto debugVar = DebugVarCarryingInst(use->getUser())) {
       assert(debugVar.getKind() == DebugVarCarryingInst::Kind::DebugValue);
       resultingString += debugVar.getName();
@@ -72,28 +75,27 @@ static void getVariableNameForValue(MarkMustCheckInst *mmci,
   }
 
   // Otherwise, we need to look at our mark_must_check's operand.
-  StackList<SILInstruction *> variableNamePath(mmci->getFunction());
-  SILValue value = mmci->getOperand();
+  StackList<SILInstruction *> variableNamePath(value2->getFunction());
   while (true) {
-    if (auto *allocInst = dyn_cast<AllocationInst>(value)) {
+    if (auto *allocInst = dyn_cast<AllocationInst>(searchValue)) {
       variableNamePath.push_back(allocInst);
       break;
     }
 
-    if (auto *globalAddrInst = dyn_cast<GlobalAddrInst>(value)) {
+    if (auto *globalAddrInst = dyn_cast<GlobalAddrInst>(searchValue)) {
       variableNamePath.push_back(globalAddrInst);
       break;
     }
 
-    if (auto *rei = dyn_cast<RefElementAddrInst>(value)) {
+    if (auto *rei = dyn_cast<RefElementAddrInst>(searchValue)) {
       variableNamePath.push_back(rei);
-      value = rei->getOperand();
+      searchValue = rei->getOperand();
       continue;
     }
 
     // If we do not do an exact match, see if we can find a debug_var inst. If
     // we do, we always break since we have a root value.
-    if (auto *use = getSingleDebugUse(value)) {
+    if (auto *use = getSingleDebugUse(searchValue)) {
       if (auto debugVar = DebugVarCarryingInst(use->getUser())) {
         assert(debugVar.getKind() == DebugVarCarryingInst::Kind::DebugValue);
         variableNamePath.push_back(use->getUser());
@@ -103,9 +105,9 @@ static void getVariableNameForValue(MarkMustCheckInst *mmci,
 
     // Otherwise, try to see if we have a single value instruction we can look
     // through.
-    if (isa<BeginBorrowInst>(value) || isa<LoadInst>(value) ||
-        isa<BeginAccessInst>(value) || isa<MarkMustCheckInst>(value)) {
-      value = cast<SingleValueInstruction>(value)->getOperand(0);
+    if (isa<BeginBorrowInst>(searchValue) || isa<LoadInst>(searchValue) ||
+        isa<BeginAccessInst>(searchValue) || isa<MarkMustCheckInst>(searchValue)) {
+      searchValue = cast<SingleValueInstruction>(searchValue)->getOperand(0);
       continue;
     }
 
@@ -132,6 +134,19 @@ static void getVariableNameForValue(MarkMustCheckInst *mmci,
   }
 }
 
+
+static void getVariableNameForValue(MarkMustCheckInst *mmci,
+                                    SmallString<64> &resultingString) {
+  return getVariableNameForValue(mmci, mmci->getOperand(), resultingString);
+}
+
+#if 0
+static void getVariableNameForValue(SILValue value,
+                                    SmallString<64> &resultingString) {
+  return getVariableNameForValue(value, value, resultingString);
+}
+#endif
+
 //===----------------------------------------------------------------------===//
 //                           MARK: Misc Diagnostics
 //===----------------------------------------------------------------------===//
diff --git a/test/SILGen/moveonly.swift b/test/SILGen/moveonly.swift
@@ -44,6 +44,9 @@ public enum NonTrivialEnum {
     case third(NonTrivialStruct)
 }
 
+var varGlobal = NonTrivialStruct()
+let letGlobal = NonTrivialStruct()
+
 public func borrowVal(_ e : NonTrivialEnum) {}
 public func borrowVal(_ e : FD) {}
 public func borrowVal(_ k: CopyableKlass) {}
@@ -517,3 +520,123 @@ func enumSwitchTest1(_ e: EnumSwitchTests.E) {
         break
     }
 }
+
+//////////////////////
+// Global Addr Test //
+//////////////////////
+
+// Make sure that we emit a new global_addr for each use.
+// CHECK-LABEL: sil hidden [ossa] @$s8moveonly16testGlobalBorrowyyF : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load_borrow [[ACCESS]]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+// CHECK: end_borrow [[LOADED_VAL]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[GEP:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK: [[LOADED_VAL:%.*]] = load_borrow [[GEP]]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+// CHECK: end_borrow [[LOADED_VAL]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+// CHECK: destroy_value [[LOADED_VAL]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]]
+// CHECK: [[LOADED_BORROWED_VAL:%.*]] = begin_borrow [[LOADED_VAL]]
+// CHECK: [[LOADED_GEP:%.*]] = struct_extract [[LOADED_BORROWED_VAL]]
+// CHECK: [[LOADED_GEP_COPY:%.*]] = copy_value [[LOADED_GEP]]
+// CHECK: end_borrow [[LOADED_BORROWED_VAL]]
+// CHECK: destroy_value [[LOADED_VAL]]
+// CHECK: apply {{%.*}}([[LOADED_GEP_COPY]])
+// CHECK: destroy_value [[LOADED_GEP_COPY]]
+// CHECK: } // end sil function '$s8moveonly16testGlobalBorrowyyF'
+func testGlobalBorrow() {
+    borrowVal(varGlobal)
+    borrowVal(varGlobal.nonTrivialStruct2)
+    borrowVal(letGlobal)
+    borrowVal(letGlobal.nonTrivialStruct2)
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s8moveonly17testGlobalConsumeyyF : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [deinit] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load [take]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [deinit] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[GEP:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK: [[LOADED_VAL:%.*]] = load [take] [[GEP]]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]]
+// CHECK: apply {{%.*}}([[LOADED_VAL]])
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
+// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]]
+// CHECK: [[LOADED_BORROWED_VAL:%.*]] = begin_borrow [[LOADED_VAL]]
+// CHECK: [[LOADED_GEP:%.*]] = struct_extract [[LOADED_BORROWED_VAL]]
+// CHECK: [[LOADED_GEP_COPY:%.*]] = copy_value [[LOADED_GEP]]
+// CHECK: end_borrow [[LOADED_BORROWED_VAL]]
+// CHECK: destroy_value [[LOADED_VAL]]
+// CHECK: apply {{%.*}}([[LOADED_GEP_COPY]])
+//
+// CHECK: } // end sil function '$s8moveonly17testGlobalConsumeyyF'
+func testGlobalConsume() {
+    consumeVal(varGlobal)
+    consumeVal(varGlobal.nonTrivialStruct2)
+    consumeVal(letGlobal)
+    consumeVal(letGlobal.nonTrivialStruct2)
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s8moveonly16testGlobalAssignyyF : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: assign {{%.*}} to [[ACCESS]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: assign {{%.*}} to [[ACCESS]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[GEP:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK: assign {{%.*}} to [[GEP]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
+// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [assignable_but_not_consumable] [[GLOBAL]]
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[MARKED_GLOBAL]]
+// CHECK: [[GEP:%.*]] = struct_element_addr [[ACCESS]]
+// CHECK: assign {{%.*}} to [[GEP]]
+// CHECK: end_access [[ACCESS]]
+// CHECK: } // end sil function '$s8moveonly16testGlobalAssignyyF'
+func testGlobalAssign() {
+    varGlobal = NonTrivialStruct()
+    varGlobal = NonTrivialStruct()
+    varGlobal.nonTrivialStruct2 = NonTrivialStruct2()
+    varGlobal.nonTrivialStruct2 = NonTrivialStruct2()
+}
diff --git a/test/SILOptimizer/moveonly_addresschecker.sil b/test/SILOptimizer/moveonly_addresschecker.sil
@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -sil-move-only-address-checker -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all %s | %FileCheck %s
+// RUN: %target-sil-opt -module-name moveonly_addresschecker -sil-move-only-address-checker -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all %s | %FileCheck %s
 
 sil_stage raw
 
@@ -39,6 +39,8 @@ public struct NonTrivialStruct2 {
     var copyableKlass = CopyableKlass()
 }
 
+sil @useNonTrivialStruct2 : $@convention(thin) (@guaranteed NonTrivialStruct2) -> ()
+
 @_moveOnly struct MyBufferView<T> {
   var ptr: UnsafeBufferPointer<T>
 }
@@ -47,6 +49,11 @@ final class ClassContainingMoveOnly {
   var value = NonTrivialStruct()
 }
 
+@_hasStorage @_hasInitialValue var varGlobal: NonTrivialStruct { get set }
+@_hasStorage @_hasInitialValue let letGlobal: NonTrivialStruct { get }
+sil_global hidden @$s23moveonly_addresschecker9varGlobalAA16NonTrivialStructVvp : $NonTrivialStruct
+sil_global hidden [let] @$s23moveonly_addresschecker9letGlobalAA16NonTrivialStructVvp : $NonTrivialStruct
+
 ///////////
 // Tests //
 ///////////
@@ -347,8 +354,8 @@ bb0(%0 : $UnsafeBufferPointer<T>, %1 : $*MyBufferView<T>):
   return %9 : $()
 }
 
-// Just make sure that we accept this and do not crash. There isn't any
-// transformation to perform.
+// No transformation here, just make sure that we treat the mark_must_check as
+// the def so that we don't crash.
 sil [ossa] @test_ref_element_addr_borrow_use : $@convention(thin) (@guaranteed ClassContainingMoveOnly) -> () {
 bb0(%0 : @guaranteed $ClassContainingMoveOnly):
   %f = function_ref @useNonTrivialStruct : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
@@ -361,4 +368,43 @@ bb0(%0 : @guaranteed $ClassContainingMoveOnly):
   end_access %3 : $*NonTrivialStruct
   %9999 = tuple()
   return %9999 : $()
-}
+}
+
+// No transformation here, just make sure that we treat the mark_must_check as
+// the def so that we don't crash.
+sil [ossa] @test_global_addr_borrow_use : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @$s23moveonly_addresschecker9varGlobalAA16NonTrivialStructVvp : $*NonTrivialStruct
+  %1 = mark_must_check [no_consume_or_assign] %0 : $*NonTrivialStruct
+  %2 = begin_access [read] [dynamic] %1 : $*NonTrivialStruct
+  %3 = load_borrow %2 : $*NonTrivialStruct
+  %4 = function_ref @useNonTrivialStruct : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
+  %5 = apply %4(%3) : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
+  end_borrow %3 : $NonTrivialStruct
+  end_access %2 : $*NonTrivialStruct
+  %8 = tuple ()
+  return %8 : $()
+}
+
+// Make sure that we do not emit any errors here.
+sil [ossa] @test_global_addr_multiple_borrow_uses : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @$s23moveonly_addresschecker9varGlobalAA16NonTrivialStructVvp : $*NonTrivialStruct
+  %1 = mark_must_check [no_consume_or_assign] %0 : $*NonTrivialStruct
+  %2 = begin_access [read] [dynamic] %1 : $*NonTrivialStruct
+  %3 = load_borrow %2 : $*NonTrivialStruct
+  %4 = function_ref @useNonTrivialStruct : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
+  %5 = apply %4(%3) : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
+  end_borrow %3 : $NonTrivialStruct
+  end_access %2 : $*NonTrivialStruct
+  %0a = global_addr @$s23moveonly_addresschecker9varGlobalAA16NonTrivialStructVvp : $*NonTrivialStruct
+  %8 = begin_access [read] [dynamic] %0a : $*NonTrivialStruct
+  %9 = struct_element_addr %8 : $*NonTrivialStruct, #NonTrivialStruct.nonTrivialStruct2
+  %10 = load_borrow %9 : $*NonTrivialStruct2
+  %11 = function_ref @useNonTrivialStruct2 : $@convention(thin) (@guaranteed NonTrivialStruct2) -> ()
+  %12 = apply %11(%10) : $@convention(thin) (@guaranteed NonTrivialStruct2) -> ()
+  end_borrow %10 : $NonTrivialStruct2
+  end_access %8 : $*NonTrivialStruct
+  %15 = tuple ()
+  return %15 : $()
+}
diff --git a/test/SILOptimizer/moveonly_addresschecker_diagnostics.swift b/test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
