[move-only-object] Make sure that we also convert copy_value -> explicit_copy_value for pure move only values along side no implicit copy values.

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -22,13 +22,24 @@
 using namespace swift;
 using namespace swift::siloptimizer;
 
+static llvm::cl::opt<bool> SilentlyEmitDiagnostics(
+    "move-only-diagnostics-silently-emit-diagnostics",
+    llvm::cl::desc(
+        "For testing purposes, emit the diagnostic silently so we can "
+        "filecheck the result of emitting an error from the move checkers"),
+    llvm::cl::init(false));
+
 //===----------------------------------------------------------------------===//
 //                              MARK: Utilities
 //===----------------------------------------------------------------------===//
 
 template <typename... T, typename... U>
 static void diagnose(ASTContext &Context, SourceLoc loc, Diag<T...> diag,
                      U &&...args) {
+  // If for testing reasons we want to return that we emitted an error but not
+  // emit the actual error itself, return early.
+  if (SilentlyEmitDiagnostics)
+    return;
   Context.Diags.diagnose(loc, diag, std::forward<U>(args)...);
 }
 

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -92,7 +92,7 @@ bool MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
       auto *cvi = dyn_cast<CopyValueInst>(&*ii);
       ++ii;
 
-      if (!cvi || !cvi->getOperand()->getType().isMoveOnlyWrapped())
+      if (!cvi || !cvi->getOperand()->getType().isMoveOnly())
         continue;
 
       SILBuilderWithScope b(cvi);
@@ -326,6 +326,9 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
   // First search for candidates to process and emit diagnostics on any
   // mark_must_check [noimplicitcopy] we didn't recognize.
   bool emittedDiagnostic = searchForCandidateMarkMustChecks(diagnosticEmitter);
+  LLVM_DEBUG(llvm::dbgs()
+             << "Emitting diagnostic when checking for mark must check inst: "
+             << (emittedDiagnostic ? "yes" : "no") << '\n');
 
   // 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
@@ -354,6 +357,9 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
     // First canonicalize ownership.
     if (!canonicalizer.canonicalize(markedValue)) {
       diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(markedValue);
+      LLVM_DEBUG(
+          llvm::dbgs()
+          << "Emitted checker doesnt understand diagnostic! Exiting early!\n");
       continue;
     } else {
       // Always set changed to true if we succeeded in canonicalizing since we
@@ -382,6 +388,8 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
   }
 
   emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
+  LLVM_DEBUG(llvm::dbgs() << "Emitting checker based diagnostic: "
+                          << (emittedDiagnostic ? "yes" : "no") << '\n');
 
   // 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
@@ -473,6 +481,17 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
             .check(accessBlockAnalysis, domTree)) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
+
+    if (getOptions().VerifyAll) {
+      for (auto &block : *getFunction()) {
+        for (auto &inst : block) {
+          if (auto *cvi = dyn_cast<CopyValueInst>(&inst)) {
+            assert(!cvi->getOperand()->getType().isMoveOnly() &&
+                   "Shouldn't copy move only types at this point");
+          }
+        }
+      }
+    }
   }
 };
 

test/SILOptimizer/moveonly_objectchecker.sil

@@ -0,0 +1,50 @@
+// RUN: %target-sil-opt -sil-move-only-object-checker -enable-experimental-move-only -enable-sil-verify-all -move-only-diagnostics-silently-emit-diagnostics %s | %FileCheck %s
+
+sil_stage raw
+
+@_moveOnly
+class Klass {}
+
+sil @classUseMoveOnlyWithoutEscaping : $@convention(thin) (@guaranteed Klass) -> ()
+
+// CHECK-LABEL: sil [ossa] @chainErrorTest : $@convention(thin) (@guaranteed Klass) -> () {
+// CHECK-NOT: copy_value
+// CHECK: explicit_copy_value
+// CHECK-NOT: copy_value
+// CHECK: explicit_copy_value
+// CHECK-NOT: copy_value
+// CHECK: } // end sil function 'chainErrorTest'
+sil [ossa] @chainErrorTest : $@convention(thin) (@guaranteed Klass) -> () {
+bb0(%0 : @guaranteed $Klass):
+  %1 = copy_value %0 : $Klass
+  %2 = mark_must_check [no_copy] %1 : $Klass
+  debug_value %2 : $Klass, let, name "x", argno 1
+  %4 = copy_value %2 : $Klass
+  %5 = move_value [lexical] %4 : $Klass
+  %6 = mark_must_check [no_implicit_copy] %5 : $Klass
+  debug_value %6 : $Klass, let, name "x2"
+  %8 = copy_value %6 : $Klass
+  %9 = move_value [lexical] %8 : $Klass
+  %10 = mark_must_check [no_implicit_copy] %9 : $Klass
+  debug_value %10 : $Klass, let, name "y2"
+  %12 = copy_value %10 : $Klass
+  %13 = move_value [lexical] %12 : $Klass
+  %14 = mark_must_check [no_implicit_copy] %13 : $Klass
+  debug_value %14 : $Klass, let, name "k2"
+  %16 = copy_value %6 : $Klass
+  %17 = move_value [lexical] %16 : $Klass
+  %18 = mark_must_check [no_implicit_copy] %17 : $Klass
+  debug_value %18 : $Klass, let, name "k3"
+  %20 = copy_value %18 : $Klass
+  %21 = move_value %20 : $Klass
+  destroy_value %21 : $Klass
+  %23 = function_ref @classUseMoveOnlyWithoutEscaping : $@convention(thin) (@guaranteed Klass) -> ()
+  %24 = apply %23(%14) : $@convention(thin) (@guaranteed Klass) -> ()
+  destroy_value %18 : $Klass
+  destroy_value %14 : $Klass
+  destroy_value %10 : $Klass
+  destroy_value %6 : $Klass
+  destroy_value %2 : $Klass
+  %30 = tuple ()
+  return %30 : $()
+}