Merge pull request #62631 from gottesmm/pr-d0e166cdd030c3826f8abfc0081e61545623320f

[move-only] Make sure that we convert copy_value/load [copy] move only to their after error variant if we error early.

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

@@ -391,7 +391,17 @@ struct MoveOnlyChecker {
   /// [noimplicitcopy]. If we find one that does not fit a pattern that we
   /// understand, emit an error diagnostic telling the programmer that the move
   /// checker did not know how to recognize this code pattern.
-  void searchForCandidateMarkMustChecks();
+  ///
+  /// Returns true if we emitted a diagnostic. Returns false otherwise.
+  bool searchForCandidateMarkMustChecks();
+
+  /// After we have emitted a diagnostic, we need to clean up the instruction
+  /// stream by converting /all/ copies of move only typed things to use
+  /// explicit_copy_value so that we maintain the SIL invariant that in
+  /// canonical SIL move only types are not copied by normal copies.
+  ///
+  /// Returns true if we actually changed any instructions.
+  void cleanupAfterEmittingDiagnostic();
 
   /// Emits an error diagnostic for \p markedValue.
   void performObjectCheck(MarkMustCheckInst *markedValue);
@@ -1334,7 +1344,44 @@ bool GlobalDataflow::compute() {
 //                       MARK: Main Pass Implementation
 //===----------------------------------------------------------------------===//
 
-void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
+void MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
+  for (auto &block : *fn) {
+    for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
+      auto *inst = &*ii;
+      ++ii;
+
+      // Convert load [copy] -> load_borrow + explicit_copy_value.
+      if (auto *li = dyn_cast<LoadInst>(inst)) {
+        if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
+          SILBuilderWithScope builder(li);
+          auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
+          auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
+          builder.createEndBorrow(li->getLoc(), lbi);
+          li->replaceAllUsesWith(cvi);
+          li->eraseFromParent();
+          changed = true;
+        }
+      }
+
+      // Convert copy_addr !take of src to its explicit value form so we don't
+      // error.
+      if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
+        if (!copyAddr->isTakeOfSrc()) {
+          SILBuilderWithScope builder(copyAddr);
+          builder.createExplicitCopyAddr(
+              copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
+              IsTake_t(copyAddr->isTakeOfSrc()),
+              IsInitialization_t(copyAddr->isInitializationOfDest()));
+          copyAddr->eraseFromParent();
+          changed = true;
+        }
+      }
+    }
+  }
+}
+
+bool MoveOnlyChecker::searchForCandidateMarkMustChecks() {
+  bool emittedDiagnostic = false;
   for (auto &block : *fn) {
     for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
       auto *mmci = dyn_cast<MarkMustCheckInst>(&*ii);
@@ -1351,6 +1398,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
               llvm::dbgs()
               << "Early emitting diagnostic for unsupported alloc box!\n");
           diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
+          emittedDiagnostic = true;
           continue;
         }
 
@@ -1360,6 +1408,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
                 llvm::dbgs()
                 << "Early emitting diagnostic for unsupported alloc box!\n");
             diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
+            emittedDiagnostic = true;
             continue;
           }
         }
@@ -1368,6 +1417,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
       moveIntroducersToProcess.insert(mmci);
     }
   }
+  return emittedDiagnostic;
 }
 
 bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
@@ -1566,15 +1616,18 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
 bool MoveOnlyChecker::check() {
   // First search for candidates to process and emit diagnostics on any
   // mark_must_check [noimplicitcopy] we didn't recognize.
-  searchForCandidateMarkMustChecks();
+  bool emittedDiagnostic = searchForCandidateMarkMustChecks();
 
   // If we didn't find any introducers to check, just return changed.
   //
   // NOTE: changed /can/ be true here if we had any mark_must_check
   // [noimplicitcopy] that we didn't understand and emitting a diagnostic upon
   // and then deleting.
-  if (moveIntroducersToProcess.empty())
+  if (moveIntroducersToProcess.empty()) {
+    if (emittedDiagnostic)
+      cleanupAfterEmittingDiagnostic();
     return changed;
+  }
 
   for (auto *markedValue : moveIntroducersToProcess) {
     LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *markedValue);
@@ -1587,7 +1640,7 @@ bool MoveOnlyChecker::check() {
       diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(markedValue);
     }
   }
-  bool emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
+  emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
 
   // Ok, now that we have performed our checks, we need to eliminate all mark
   // must check inst since it is invalid for these to be in canonical SIL and
@@ -1608,40 +1661,7 @@ bool MoveOnlyChecker::check() {
   // It is also ok that we use a little more compile time and go over the
   // function again, since we are going to fail the compilation and not codegen.
   if (emittedDiagnostic) {
-    for (auto &block : *fn) {
-      for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
-        auto *inst = &*ii;
-        ++ii;
-
-        // Convert load [copy] -> load_borrow + explicit_copy_value.
-        if (auto *li = dyn_cast<LoadInst>(inst)) {
-          if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
-            SILBuilderWithScope builder(li);
-            auto *lbi =
-                builder.createLoadBorrow(li->getLoc(), li->getOperand());
-            auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
-            builder.createEndBorrow(li->getLoc(), lbi);
-            li->replaceAllUsesWith(cvi);
-            li->eraseFromParent();
-            changed = true;
-          }
-        }
-
-        // Convert copy_addr !take of src to its explicit value form so we don't
-        // error.
-        if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
-          if (!copyAddr->isTakeOfSrc()) {
-            SILBuilderWithScope builder(copyAddr);
-            builder.createExplicitCopyAddr(
-                copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
-                IsTake_t(copyAddr->isTakeOfSrc()),
-                IsInitialization_t(copyAddr->isInitializationOfDest()));
-            copyAddr->eraseFromParent();
-            changed = true;
-          }
-        }
-      }
-    }
+    cleanupAfterEmittingDiagnostic();
   }
 
   return changed;

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -73,10 +73,39 @@ struct MoveOnlyChecker {
 
   bool check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
              DominanceInfo *domTree);
+
+  /// After we have emitted a diagnostic, we need to clean up the instruction
+  /// stream by converting /all/ copies of move only typed things to use
+  /// explicit_copy_value so that we maintain the SIL invariant that in
+  /// canonical SIL move only types are not copied by normal copies.
+  ///
+  /// Returns true if we actually changed any instructions.
+  bool cleanupAfterEmittingDiagnostic();
 };
 
 } // namespace
 
+bool MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
+  bool changed = false;
+  for (auto &block : *fn) {
+    for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
+      auto *cvi = dyn_cast<CopyValueInst>(&*ii);
+      ++ii;
+
+      if (!cvi || !cvi->getOperand()->getType().isMoveOnlyWrapped())
+        continue;
+
+      SILBuilderWithScope b(cvi);
+      auto *expCopy =
+          b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
+      cvi->replaceAllUsesWith(expCopy);
+      cvi->eraseFromParent();
+      changed = true;
+    }
+  }
+  return changed;
+}
+
 bool MoveOnlyChecker::searchForCandidateMarkMustChecks(
     DiagnosticEmitter &emitter) {
   bool changed = false;
@@ -296,15 +325,22 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
 
   // First search for candidates to process and emit diagnostics on any
   // mark_must_check [noimplicitcopy] we didn't recognize.
-  changed |= searchForCandidateMarkMustChecks(diagnosticEmitter);
+  bool emittedDiagnostic = searchForCandidateMarkMustChecks(diagnosticEmitter);
 
-  // If we didn't find any introducers to check, just return changed.
+  // If we didn't find any introducers to check, just return if we emitted an
+  // error (which is the only way we emitted a change to the instruction
+  // stream).
   //
   // NOTE: changed /can/ be true here if we had any mark_must_check
   // [noimplicitcopy] that we didn't understand and emitting a diagnostic upon
   // and then deleting.
-  if (moveIntroducersToProcess.empty())
-    return changed;
+  if (moveIntroducersToProcess.empty()) {
+    if (emittedDiagnostic) {
+      cleanupAfterEmittingDiagnostic();
+      return true;
+    }
+    return false;
+  }
 
   auto moveIntroducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
                                             moveIntroducersToProcess.end());
@@ -345,7 +381,7 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
     diagnosticEmitter.emitObjectOwnedDiagnostic(markedValue);
   }
 
-  bool emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
+  emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
 
   // Ok, we have success. All of our marker instructions were proven as safe or
   // we emitted a diagnostic. Now we need to clean up the IR by eliminating our
@@ -390,30 +426,15 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
   }
 
   // Once we have finished processing, if we emitted any diagnostics, then we
-  // may have copy_value of @moveOnly typed values. This is not valid in
-  // Canonical SIL, so we need to ensure that those copy_value become
-  // explicit_copy_value. This is ok to do since we are already going to fail
-  // the compilation and just are trying to maintain SIL invariants.
+  // may have copy_value of move only and @moveOnly wrapped type values. This is
+  // not valid in Canonical SIL, so we need to ensure that those copy_value
+  // become explicit_copy_value. This is ok to do since we are already going to
+  // fail the compilation and just are trying to maintain SIL invariants.
   //
   // It is also ok that we use a little more compile time and go over the
   // function again, since we are going to fail the compilation and not codegen.
   if (emittedDiagnostic) {
-    for (auto &block : *fn) {
-      for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
-        auto *cvi = dyn_cast<CopyValueInst>(&*ii);
-        ++ii;
-
-        if (!cvi || !cvi->getOperand()->getType().isMoveOnlyWrapped())
-          continue;
-
-        SILBuilderWithScope b(cvi);
-        auto *expCopy =
-            b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
-        cvi->replaceAllUsesWith(expCopy);
-        cvi->eraseFromParent();
-        changed = true;
-      }
-    }
+    changed |= cleanupAfterEmittingDiagnostic();
   }
 
   return changed;