Merge pull request #41085 from gottesmm/pr-f777a3678aa7c0fcd9369e7ad04c5d94a067024a

gottesmm · web-flow · commit 5ced1d17eff8 · 2022-01-29T19:23:51.000-08:00
[no-implicit-copy] Add a new instruction called MarkMustCheckInst and use it in the move checker.
diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def
@@ -653,6 +653,10 @@ ERROR(sil_keypath_no_use_of_operand_in_pattern,none,
       (unsigned))
 ERROR(sil_movevalue_invalid_optional_attribute,none,
       "Optional attribute '[%0]' can not be applied to move_value", (StringRef))
+ERROR(sil_markmustcheck_invalid_attribute,none,
+      "Attribute '[%0]' can not be applied to mark_value_as_moveonly", (StringRef))
+ERROR(sil_markmustcheck_requires_attribute,none,
+      "mark_must_check requires an attribute like 'noImplicitCopy'", ())
 
 // SIL Basic Blocks
 ERROR(expected_sil_block_name,none,
diff --git a/include/swift/SIL/SILBuilder.h b/include/swift/SIL/SILBuilder.h
@@ -1279,6 +1279,13 @@ class SILBuilder {
         getSILDebugLocation(loc), srcAddr, takeAddr));
   }
 
+  MarkMustCheckInst *
+  createMarkMustCheckInst(SILLocation loc, SILValue src,
+                          MarkMustCheckInst::CheckKind kind) {
+    return insert(new (getModule())
+                      MarkMustCheckInst(getSILDebugLocation(loc), src, kind));
+  }
+
   UnconditionalCheckedCastInst *
   createUnconditionalCheckedCast(SILLocation Loc, SILValue op,
                                  SILType destLoweredTy,
diff --git a/include/swift/SIL/SILCloner.h b/include/swift/SIL/SILCloner.h
@@ -1790,6 +1790,15 @@ void SILCloner<ImplClass>::visitMoveValueInst(MoveValueInst *Inst) {
   recordClonedInstruction(Inst, MVI);
 }
 
+template <typename ImplClass>
+void SILCloner<ImplClass>::visitMarkMustCheckInst(MarkMustCheckInst *Inst) {
+  getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
+  auto *MVI = getBuilder().createMarkMustCheckInst(
+      getOpLocation(Inst->getLoc()), getOpValue(Inst->getOperand()),
+      Inst->getCheckKind());
+  recordClonedInstruction(Inst, MVI);
+}
+
 template <typename ImplClass>
 void SILCloner<ImplClass>::visitReleaseValueInst(ReleaseValueInst *Inst) {
   getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h
@@ -7501,6 +7501,38 @@ class MarkUnresolvedMoveAddrInst
   MutableArrayRef<Operand> getAllOperands() { return Operands.asArray(); }
 };
 
+/// This is a marker instruction that has no effect that is consumed by a
+/// diagnostic based semantic checker. Example: no implicit copy. Only legal in
+/// Raw SIL so that we can guarantee canonical SIL has had all SSA based
+/// checking by the checkers that rely upon this instruction.
+class MarkMustCheckInst
+    : public UnaryInstructionBase<SILInstructionKind::MarkMustCheckInst,
+                                  SingleValueInstruction>,
+      public OwnershipForwardingMixin {
+  friend class SILBuilder;
+
+public:
+  enum class CheckKind : unsigned {
+    Invalid = 0,
+    NoImplicitCopy,
+  };
+
+private:
+  CheckKind kind;
+
+  MarkMustCheckInst(SILDebugLocation DebugLoc, SILValue operand,
+                    CheckKind checkKind)
+      : UnaryInstructionBase(DebugLoc, operand, operand->getType()),
+        OwnershipForwardingMixin(SILInstructionKind::MarkMustCheckInst,
+                                 operand->getOwnershipKind()),
+        kind(checkKind) {}
+
+public:
+  CheckKind getCheckKind() const { return kind; }
+
+  bool isNoImplicitCopy() const { return kind == CheckKind::NoImplicitCopy; }
+};
+
 /// Given an object reference, return true iff it is non-nil and refers
 /// to a native swift object with strong reference count of 1.
 class IsUniqueInst
@@ -9683,7 +9715,8 @@ inline bool OwnershipForwardingMixin::isa(SILInstructionKind kind) {
          OwnershipForwardingTermInst::classof(kind) ||
          OwnershipForwardingConversionInst::classof(kind) ||
          OwnershipForwardingSelectEnumInstBase::classof(kind) ||
-         OwnershipForwardingMultipleValueInstruction::classof(kind);
+         OwnershipForwardingMultipleValueInstruction::classof(kind) ||
+         kind == SILInstructionKind::MarkMustCheckInst;
 }
 
 inline OwnershipForwardingMixin *
@@ -9705,6 +9738,8 @@ OwnershipForwardingMixin::get(SILInstruction *inst) {
   if (auto *result =
           dyn_cast<OwnershipForwardingMultipleValueInstruction>(inst))
     return result;
+  if (auto *result = dyn_cast<MarkMustCheckInst>(inst))
+    return result;
   return nullptr;
 }
 
diff --git a/include/swift/SIL/SILNodes.def b/include/swift/SIL/SILNodes.def
@@ -475,7 +475,12 @@ ABSTRACT_VALUE_AND_INST(SingleValueInstruction, ValueBase, SILInstruction)
   // effects relative to other OSSA values like copy_value.
   SINGLE_VALUE_INST(MoveValueInst, move_value, SingleValueInstruction, None,
                     DoesNotRelease)
-  
+  // A canary value inserted by a SIL generating frontend to signal to the move
+  // checker to check a specific value.  Valid only in Raw SIL. The relevant
+  // checkers should remove the mark_must_check instruction after successfully
+  // running the relevant diagnostic.
+  SINGLE_VALUE_INST(MarkMustCheckInst, mark_must_check,
+                    SingleValueInstruction, None, DoesNotRelease)
   // A move_addr is a Raw SIL only instruction that is equivalent to a copy_addr
   // [init]. It is lowered during the diagnostic passes to a copy_addr [init] if
   // the move checker found uses that prevented us from converting this to a
diff --git a/lib/IRGen/IRGenSIL.cpp b/lib/IRGen/IRGenSIL.cpp
@@ -1154,6 +1154,9 @@ class IRGenSILFunction :
     auto e = getLoweredExplosion(i->getOperand());
     setLoweredExplosion(i, e);
   }
+  void visitMarkMustCheckInst(MarkMustCheckInst *i) {
+    llvm_unreachable("Invalid in Lowered SIL");
+  }
   void visitReleaseValueInst(ReleaseValueInst *i);
   void visitReleaseValueAddrInst(ReleaseValueAddrInst *i);
   void visitDestroyValueInst(DestroyValueInst *i);
diff --git a/lib/SIL/IR/OperandOwnership.cpp b/lib/SIL/IR/OperandOwnership.cpp
@@ -327,6 +327,7 @@ FORWARDING_OWNERSHIP(UnconditionalCheckedCast)
 FORWARDING_OWNERSHIP(InitExistentialRef)
 FORWARDING_OWNERSHIP(DifferentiableFunction)
 FORWARDING_OWNERSHIP(LinearFunction)
+FORWARDING_OWNERSHIP(MarkMustCheck)
 #undef FORWARDING_OWNERSHIP
 
 // Arbitrary value casts are forwarding instructions that are also allowed to
diff --git a/lib/SIL/IR/SILPrinter.cpp b/lib/SIL/IR/SILPrinter.cpp
@@ -1901,6 +1901,18 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
     *this << getIDAndType(I->getOperand());
   }
 
+  void visitMarkMustCheckInst(MarkMustCheckInst *I) {
+    using CheckKind = MarkMustCheckInst::CheckKind;
+    switch (I->getCheckKind()) {
+    case CheckKind::Invalid:
+      llvm_unreachable("Invalid?!");
+    case CheckKind::NoImplicitCopy:
+      *this << "[no_implicit_copy] ";
+      break;
+    }
+    *this << getIDAndType(I->getOperand());
+  }
+
 #define UNCHECKED_REF_STORAGE(Name, ...)                                       \
   void visitStrongCopy##Name##ValueInst(StrongCopy##Name##ValueInst *I) {      \
     *this << getIDAndType(I->getOperand());                                    \
diff --git a/lib/SIL/IR/ValueOwnership.cpp b/lib/SIL/IR/ValueOwnership.cpp
@@ -277,6 +277,7 @@ FORWARDING_OWNERSHIP_INST(MarkDependence)
 FORWARDING_OWNERSHIP_INST(InitExistentialRef)
 FORWARDING_OWNERSHIP_INST(DifferentiableFunction)
 FORWARDING_OWNERSHIP_INST(LinearFunction)
+FORWARDING_OWNERSHIP_INST(MarkMustCheck)
 #undef FORWARDING_OWNERSHIP_INST
 
 ValueOwnershipKind
diff --git a/lib/SIL/Parser/ParseSIL.cpp b/lib/SIL/Parser/ParseSIL.cpp
@@ -13,6 +13,7 @@
 #include "SILParserFunctionBuilder.h"
 #include "SILParserState.h"
 #include "swift/AST/ASTWalker.h"
+#include "swift/AST/DiagnosticsParse.h"
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/NameLookup.h"
@@ -3322,6 +3323,34 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
     break;
   }
 
+  case SILInstructionKind::MarkMustCheckInst: {
+    if (parseTypedValueRef(Val, B))
+      return true;
+    if (parseSILDebugLocation(InstLoc, B))
+      return true;
+
+    StringRef AttrName;
+    if (!parseSILOptional(AttrName, *this)) {
+      auto diag = diag::sil_markmustcheck_requires_attribute;
+      P.diagnose(InstLoc.getSourceLoc(), diag);
+      return true;
+    }
+
+    using CheckKind = MarkMustCheckInst::CheckKind;
+    CheckKind CKind = llvm::StringSwitch<CheckKind>(AttrName)
+                          .Case("no_implicit_copy", CheckKind::NoImplicitCopy)
+                          .Default(CheckKind::Invalid);
+
+    if (CKind == CheckKind::Invalid) {
+      auto diag = diag::sil_markmustcheck_invalid_attribute;
+      P.diagnose(InstLoc.getSourceLoc(), diag, AttrName);
+      return true;
+    }
+    auto *MVI = B.createMarkMustCheckInst(InstLoc, Val, CKind);
+    ResultVal = MVI;
+    break;
+  }
+
   case SILInstructionKind::LoadInst: {
     Optional<LoadOwnershipQualifier> Qualifier;
     SourceLoc AddrLoc;
diff --git a/lib/SIL/Utils/InstructionUtils.cpp b/lib/SIL/Utils/InstructionUtils.cpp
@@ -425,6 +425,7 @@ RuntimeEffect swift::getRuntimeEffect(SILInstruction *inst, SILType &impactType)
   case SILInstructionKind::ValueToBridgeObjectInst:
   case SILInstructionKind::MarkDependenceInst:
   case SILInstructionKind::MoveValueInst:
+  case SILInstructionKind::MarkMustCheckInst:
   case SILInstructionKind::UncheckedOwnershipConversionInst:
   case SILInstructionKind::LoadInst:
   case SILInstructionKind::LoadBorrowInst:
diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp
@@ -5382,6 +5382,12 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
             "Result and operand must have the same type, today.");
   }
 
+  void checkMarkMustCheckInst(MarkMustCheckInst *i) {
+    require(i->getModule().getStage() == SILStage::Raw,
+            "Only valid in Raw SIL! Should have been eliminated by /some/ "
+            "diagnostic pass");
+  }
+
   void verifyEpilogBlocks(SILFunction *F) {
     bool FoundReturnBlock = false;
     bool FoundThrowBlock = false;
diff --git a/lib/SILGen/SILGenDecl.cpp b/lib/SILGen/SILGenDecl.cpp
@@ -574,7 +574,8 @@ class LetValueInitialization : public Initialization {
           value = SILValue(SGF.B.createBeginBorrow(PrologueLoc, value,
                                                    /*isLexical*/ true));
           value = SGF.B.createCopyValue(PrologueLoc, value);
-          value = SGF.B.createMoveValue(PrologueLoc, value);
+          value = SGF.B.createMarkMustCheckInst(
+              PrologueLoc, value, MarkMustCheckInst::CheckKind::NoImplicitCopy);
         } else {
           value = SILValue(
               SGF.B.createBeginBorrow(PrologueLoc, value, /*isLexical*/ true));
@@ -1798,14 +1799,16 @@ void SILGenFunction::destroyLocalVariable(SILLocation silLoc, VarDecl *vd) {
   }
 
   if (getASTContext().LangOpts.EnableExperimentalMoveOnly) {
-    if (auto *mvi = dyn_cast<MoveValueInst>(Val.getDefiningInstruction())) {
-      if (auto *cvi = dyn_cast<CopyValueInst>(mvi->getOperand())) {
-        if (auto *bbi = dyn_cast<BeginBorrowInst>(cvi->getOperand())) {
-          if (bbi->isLexical()) {
-            B.emitDestroyValueOperation(silLoc, mvi);
-            B.createEndBorrow(silLoc, bbi);
-            B.emitDestroyValueOperation(silLoc, bbi->getOperand());
-            return;
+    if (auto *mvi = dyn_cast<MarkMustCheckInst>(Val.getDefiningInstruction())) {
+      if (mvi->isNoImplicitCopy()) {
+        if (auto *cvi = dyn_cast<CopyValueInst>(mvi->getOperand())) {
+          if (auto *bbi = dyn_cast<BeginBorrowInst>(cvi->getOperand())) {
+            if (bbi->isLexical()) {
+              B.emitDestroyValueOperation(silLoc, mvi);
+              B.createEndBorrow(silLoc, bbi);
+              B.emitDestroyValueOperation(silLoc, bbi->getOperand());
+              return;
+            }
           }
         }
       }
diff --git a/lib/SILGen/SILGenProlog.cpp b/lib/SILGen/SILGenProlog.cpp
@@ -22,6 +22,7 @@
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/PropertyWrappers.h"
 #include "swift/SIL/SILArgument.h"
+#include "swift/SIL/SILInstruction.h"
 
 using namespace swift;
 using namespace Lowering;
@@ -284,7 +285,8 @@ struct ArgumentInitHelper {
         SGF.Cleanups.pushCleanup<EndBorrowCleanup>(value);
         if (isNoImplicitCopy) {
           value = SGF.B.emitCopyValueOperation(loc, value);
-          value = SGF.B.createMoveValue(loc, value);
+          value = SGF.B.createMarkMustCheckInst(
+              loc, value, MarkMustCheckInst::CheckKind::NoImplicitCopy);
           SGF.enterDestroyCleanup(value);
         }
       }
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp
@@ -58,13 +58,13 @@ struct MoveOnlyChecker {
 
 bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
                             DominanceInfo *domTree) {
-  SmallSetVector<MoveValueInst *, 32> moveIntroducersToProcess;
+  SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
 
   for (auto &block : *fn) {
     for (auto &ii : block) {
-      auto *mvi = dyn_cast<MoveValueInst>(&ii);
+      auto *mvi = dyn_cast<MarkMustCheckInst>(&ii);
       // For now only handle move_only.
-      if (!mvi)
+      if (!mvi || !mvi->isNoImplicitCopy())
         continue;
       auto *cvi = dyn_cast<CopyValueInst>(mvi->getOperand());
       if (!cvi)
@@ -78,7 +78,7 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
 
   auto callbacks =
       InstModCallbacks().onDelete([&](SILInstruction *instToDelete) {
-        if (auto *mvi = dyn_cast<MoveValueInst>(instToDelete))
+        if (auto *mvi = dyn_cast<MarkMustCheckInst>(instToDelete))
           moveIntroducersToProcess.remove(mvi);
         instToDelete->eraseFromParent();
       });
@@ -99,16 +99,16 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
       domTree, deleter, foundConsumingUseNeedingCopy,
       foundConsumingUseNotNeedingCopy);
   auto &astContext = fn->getASTContext();
-  auto movesToProcess = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
-                                           moveIntroducersToProcess.end());
-  while (!movesToProcess.empty()) {
+  auto moveIntroducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
+                                            moveIntroducersToProcess.end());
+  while (!moveIntroducers.empty()) {
     SWIFT_DEFER {
       consumingUsesNeedingCopy.clear();
       consumingUsesNotNeedingCopy.clear();
     };
 
-    SILValue movedValue = movesToProcess.front();
-    movesToProcess = movesToProcess.drop_front(1);
+    SILValue movedValue = moveIntroducers.front();
+    moveIntroducers = moveIntroducers.drop_front(1);
     LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *movedValue);
     changed |= canonicalizer.canonicalizeValueLifetime(movedValue);
 
@@ -144,13 +144,16 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
     }
   }
 
-  // Now go back through all of the move_value and change their copy_value to be
-  // on the operand of the lexical begin_borrow that introduced it.
+  // Ok, we have success. All of our marker instructions were proven as
+  // safe. Now we need to clean up the IR by eliminating our marker instructions
+  // to signify that the checked SIL is correct.
+  //
+  // NOTE: This is enforced in the verifier by only allowing MarkMustCheckInst
+  // in Raw SIL.
   while (!moveIntroducersToProcess.empty()) {
     auto *mvi = moveIntroducersToProcess.pop_back_val();
-    auto *cvi = cast<CopyValueInst>(mvi->getOperand());
-    auto *bbi = cast<BeginBorrowInst>(cvi->getOperand());
-    cvi->setOperand(bbi->getOperand());
+    mvi->replaceAllUsesWith(mvi->getOperand());
+    mvi->eraseFromParent();
     changed = true;
   }
 
diff --git a/lib/SILOptimizer/UtilityPasses/SerializeSILPass.cpp b/lib/SILOptimizer/UtilityPasses/SerializeSILPass.cpp
@@ -205,6 +205,7 @@ static bool hasOpaqueArchetype(TypeExpansionContext context,
   case SILInstructionKind::CopyValueInst:
   case SILInstructionKind::ExplicitCopyValueInst:
   case SILInstructionKind::MoveValueInst:
+  case SILInstructionKind::MarkMustCheckInst:
 #define UNCHECKED_REF_STORAGE(Name, ...)                                       \
   case SILInstructionKind::StrongCopy##Name##ValueInst:
 #define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)            \
diff --git a/lib/SILOptimizer/Utils/SILInliner.cpp b/lib/SILOptimizer/Utils/SILInliner.cpp
@@ -755,6 +755,7 @@ InlineCost swift::instructionInlineCost(SILInstruction &I) {
   case SILInstructionKind::BindMemoryInst:
   case SILInstructionKind::RebindMemoryInst:
   case SILInstructionKind::MoveValueInst:
+  case SILInstructionKind::MarkMustCheckInst:
     return InlineCost::Free;
 
   // Typed GEPs are free.
diff --git a/lib/Serialization/DeserializeSIL.cpp b/lib/Serialization/DeserializeSIL.cpp
@@ -2010,6 +2010,17 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
     break;
   }
 
+  case SILInstructionKind::MarkMustCheckInst: {
+    using CheckKind = MarkMustCheckInst::CheckKind;
+    auto Ty = MF->getType(TyID);
+    auto CKind = CheckKind(Attr);
+    ResultInst = Builder.createMarkMustCheckInst(
+        Loc,
+        getLocalValue(ValID, getSILType(Ty, (SILValueCategory)TyCategory, Fn)),
+        CKind);
+    break;
+  }
+
   case SILInstructionKind::LoadInst: {
     auto Ty = MF->getType(TyID);
     auto Qualifier = LoadOwnershipQualifier(Attr);
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
@@ -56,7 +56,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 665; // opaque type param bits
+const uint16_t SWIFTMODULE_VERSION_MINOR = 666; // mark_must_check initial
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
diff --git a/lib/Serialization/SerializeSIL.cpp b/lib/Serialization/SerializeSIL.cpp
diff --git a/test/SILOptimizer/move_only_checker.swift b/test/SILOptimizer/move_only_checker.swift
