[move-only] Make sure that when the borrow to destructure transform emits an error, we clean up the IR appropriately.

This ensures we appropriately eliminate mark_must_check if we error and change
copy_value -> explicit_copy_value.

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

@@ -28,6 +28,8 @@
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/Analysis/Analysis.h"
 #include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h"
+#include "swift/Basic/BlotSetVector.h"
+#include "swift/Basic/FrozenMultiMap.h"
 #include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
@@ -37,9 +39,6 @@
 using namespace swift;
 using namespace swift::siloptimizer;
 
-using namespace swift;
-using namespace swift::siloptimizer;
-
 namespace {
 using AvailableValues = BorrowToDestructureTransform::AvailableValues;
 }
@@ -202,8 +201,8 @@ void BorrowToDestructureTransform::checkDestructureUsesOnBoundary() const {
   for (auto *use : destructureNeedingUses) {
     LLVM_DEBUG(llvm::dbgs()
                << "    DestructureNeedingUse: " << *use->getUser());
-    auto destructureUse = *TypeTreeLeafTypeRange::get(use->get(), mmci);
-    if (liveness.isWithinBoundary(use->getUser(), destructureUse)) {
+    auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+    if (liveness.isWithinBoundary(use->getUser(), destructureUseSpan)) {
       LLVM_DEBUG(llvm::dbgs() << "        Within boundary! Emitting error!\n");
       // Emit an error. We have a use after free.
       //
@@ -217,7 +216,7 @@ void BorrowToDestructureTransform::checkDestructureUsesOnBoundary() const {
           liveness.getNumSubElements());
       liveness.computeBoundary(boundary);
       diagnosticEmitter.emitObjectDestructureNeededWithinBorrowBoundary(
-          mmci, use->getUser(), destructureUse, boundary);
+          mmci, use->getUser(), destructureUseSpan, boundary);
       return;
     } else {
       LLVM_DEBUG(llvm::dbgs() << "        On boundary! No error!\n");
@@ -1338,12 +1337,20 @@ static bool runTransform(SILFunction *fn,
     auto *mmci = moveIntroducersToProcess.back();
     moveIntroducersToProcess = moveIntroducersToProcess.drop_back();
 
+    unsigned currentDiagnosticCount = diagnosticEmitter.getDiagnosticCount();
+
     StackList<BeginBorrowInst *> borrowWorklist(mmci->getFunction());
 
     // If we failed to gather borrows due to the transform not understanding
-    // part of the SIL, fail and return false.
-    if (!BorrowToDestructureTransform::gatherBorrows(mmci, borrowWorklist))
-      return madeChange;
+    // part of the SIL, emit a diagnostic, RAUW the mark must check, and
+    // continue.
+    if (!BorrowToDestructureTransform::gatherBorrows(mmci, borrowWorklist)) {
+      diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
+      mmci->replaceAllUsesWith(mmci->getOperand());
+      mmci->eraseFromParent();
+      madeChange = true;
+      continue;
+    }
 
     // If we do not have any borrows to process, continue and process the next
     // instruction.
@@ -1360,29 +1367,43 @@ static bool runTransform(SILFunction *fn,
                                            poa, discoveredBlocks);
 
     // Attempt to gather uses. Return if we saw something that we did not
-    // understand. Return made change so we invalidate as appropriate.
-    if (!transform.gatherUses(borrowWorklist))
-      return madeChange;
+    // understand. Emit a compiler did not understand diagnostic, RAUW the mmci
+    // so later passes do not see it, and set madeChange to true.
+    if (!transform.gatherUses(borrowWorklist)) {
+      diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
+      mmci->replaceAllUsesWith(mmci->getOperand());
+      mmci->eraseFromParent();
+      madeChange = true;
+      continue;
+    }
 
     // Next make sure that any destructure needing instructions are on the
     // boundary in a per bit field sensitive manner.
     transform.checkDestructureUsesOnBoundary();
 
-    // If we emitted any diagnostic, break out. We return true since we actually
-    // succeeded in our processing by finding the error. We only return false if
-    // we want to tell the rest of the checker that there was an internal
-    // compiler error that we need to emit a "compiler doesn't understand
-    // error".
-    if (diagnosticEmitter.emittedAnyDiagnostics())
-      return madeChange;
+    // If we emitted any diagnostic, set madeChange to true, eliminate our mmci,
+    // and continue.
+    if (currentDiagnosticCount != diagnosticEmitter.getDiagnosticCount()) {
+      mmci->replaceAllUsesWith(mmci->getOperand());
+      mmci->eraseFromParent();
+      madeChange = true;
+      continue;
+    }
 
     // At this point, we know that all of our destructure requiring uses are on
     // the boundary of our live range. Now we need to do the rewriting.
     transform.blockToAvailableValues.emplace(transform.liveness);
     transform.rewriteUses();
 
     // Now that we have done our rewritting, we need to do a few cleanups.
+    //
+    // NOTE: We do not eliminate our mark_must_check since we want later passes
+    // to do additional checking upon the mark_must_check including making sure
+    // that our destructures do not need cause the need for additional copies to
+    // be inserted. We only eliminate the mark_must_check if we emitted a
+    // diagnostic of some sort.
     transform.cleanup(borrowWorklist);
+    madeChange = true;
   }
 
   return madeChange;
@@ -1405,8 +1426,9 @@ class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
     assert(fn->getModule().getStage() == SILStage::Raw &&
            "Should only run on Raw SIL");
 
-    LLVM_DEBUG(llvm::dbgs() << "===> MoveOnly Object Checker. Visiting: "
-                            << fn->getName() << '\n');
+    LLVM_DEBUG(llvm::dbgs()
+               << "===> MoveOnlyBorrowToDestructureTransform. Visiting: "
+               << fn->getName() << '\n');
 
     auto *postOrderAnalysis = getAnalysis<PostOrderAnalysis>();
 
@@ -1419,14 +1441,22 @@ class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
 
-    if (emitter.emittedAnyDiagnostics())
+    if (emitter.emittedAnyDiagnostics()) {
+      if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
+        invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
       return;
+    }
 
     auto introducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
                                           moveIntroducersToProcess.end());
     if (runTransform(fn, introducers, postOrderAnalysis, emitter)) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
+
+    if (emitter.emittedAnyDiagnostics()) {
+      if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
+        invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
+    }
   }
 };
 

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

@@ -41,10 +41,13 @@ class DiagnosticEmitter {
   /// here.
   SmallPtrSet<MarkMustCheckInst *, 4> valuesWithDiagnostics;
 
-  // Track any violating uses we have emitted a diagnostic for so we don't emit
-  // multiple diagnostics for the same use.
+  /// Track any violating uses we have emitted a diagnostic for so we don't emit
+  /// multiple diagnostics for the same use.
   SmallPtrSet<SILInstruction *, 8> useWithDiagnostic;
 
+  /// A count of the total diagnostics emitted so that callers of routines that
+  /// take a diagnostic emitter can know if the emitter emitted additional
+  /// diagnosics while running a callee.
   unsigned diagnosticCount = 0;
 
 public:

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -290,6 +290,22 @@ bool swift::siloptimizer::cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(
         cvi->replaceAllUsesWith(expCopy);
         cvi->eraseFromParent();
         localChanged = true;
+        continue;
+      }
+
+      // Also eliminate any mark_must_check on objects, just to be safe. We
+      // emitted an object level diagnostic and if the user wants to get more
+      // diagnostics, they should fix these diagnostics and recompile.
+      if (auto *mmci = dyn_cast<MarkMustCheckInst>(&*ii)) {
+        ++ii;
+
+        if (mmci->getType().isAddress())
+          continue;
+
+        mmci->replaceAllUsesWith(mmci->getOperand());
+        mmci->eraseFromParent();
+        localChanged = true;
+        continue;
       }
 
       ++ii;

test/SILOptimizer/moveonly_borrow_to_destructure_transform_diagnostics.sil

@@ -0,0 +1,83 @@
+// RUN: %target-sil-opt -move-only-diagnostics-silently-emit-diagnostics -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all -sil-move-only-borrow-to-destructure %s | %FileCheck %s
+// RUN: %target-sil-opt -verify -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all -sil-move-only-borrow-to-destructure %s
+
+sil_stage raw
+
+import Builtin
+
+//////////////////
+// Declarations //
+//////////////////
+
+class Klass {}
+@_moveOnly
+class MoveOnlyKlass {
+  var value: Builtin.Int32
+}
+
+@_moveOnly
+struct KlassPair {
+  var lhs: Klass
+  var rhs: MoveOnlyKlass
+}
+
+@_moveOnly
+struct AggStruct {
+  var pair: KlassPair
+}
+
+@_moveOnly
+struct KlassPair2 {
+  var lhs: MoveOnlyKlass
+  var rhs: MoveOnlyKlass
+}
+
+@_moveOnly
+struct AggStruct2 {
+  var lhs: MoveOnlyKlass
+  var pair: KlassPair2
+  var rhs: MoveOnlyKlass
+}
+
+@_moveOnly
+struct SingleIntContainingStruct {
+    var value: Builtin.Int32
+}
+
+sil @misc_use : $@convention(thin) () -> ()
+sil @aggstruct_consume : $@convention(thin) (@owned AggStruct) -> ()
+sil @moveonlyklass_consume : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+sil @moveonlyklass_use : $@convention(thin) (@guaranteed MoveOnlyKlass) -> ()
+sil @klass_use : $@convention(thin) (@guaranteed Klass) -> ()
+
+///////////
+// Tests //
+///////////
+
+// CHECK-LABEL: sil [ossa] @test_access_single_child_field_consume : $@convention(method) (@guaranteed AggStruct2) -> () {
+// CHECK: bb0([[ARG:%.*]] : @guaranteed $AggStruct2):
+// CHECK:   explicit_copy_value [[ARG]]
+// CHECK-NOT: mark_must_check
+// CHECK: } // end sil function 'test_access_single_child_field_consume'
+sil [ossa] @test_access_single_child_field_consume : $@convention(method) (@guaranteed AggStruct2) -> () {
+bb0(%0 : @guaranteed $AggStruct2):
+  %1 = copy_value %0 : $AggStruct2
+  %2 = mark_must_check [no_copy] %1 : $AggStruct2 // expected-error {{'self' has a move only field that was consumed before later uses}}
+  debug_value %2 : $AggStruct2, let, name "self", argno 1, implicit
+  %4 = begin_borrow %2 : $AggStruct2
+  %5 = struct_extract %4 : $AggStruct2, #AggStruct2.pair
+  %6 = struct_extract %5 : $KlassPair2, #KlassPair2.lhs
+  %7 = copy_value %6 : $MoveOnlyKlass
+  end_borrow %4 : $AggStruct2
+  %4a = begin_borrow %2 : $AggStruct2
+  %5a = struct_extract %4a : $AggStruct2, #AggStruct2.pair
+  %6a = struct_extract %5a : $KlassPair2, #KlassPair2.lhs
+  %7a = copy_value %6a : $MoveOnlyKlass
+  end_borrow %4a : $AggStruct2
+  destroy_value %2 : $AggStruct2
+  %9 = function_ref @moveonlyklass_consume : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+  apply %9(%7) : $@convention(thin) (@owned MoveOnlyKlass) -> () // expected-note {{consuming use}}
+  apply %9(%7a) : $@convention(thin) (@owned MoveOnlyKlass) -> () // expected-note {{boundary use here}}
+  %9999 = tuple()
+  return %9999 : $()
+}

test/SILOptimizer/moveonly_objectchecker_diagnostics.swift

@@ -949,8 +949,8 @@ public func aggStructAccessFieldOwnedArg(_ x2: __owned AggStruct) {
     }
 }
 
-public func aggStructConsumeField(_ x: AggStruct) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggStructConsumeField(_ x: AggStruct) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.lhs)
     for _ in 0..<1024 {
@@ -996,8 +996,8 @@ public func aggStructAccessGrandFieldOwnedArg(_ x2: __owned AggStruct) {
     }
 }
 
-public func aggStructConsumeGrandField(_ x: AggStruct) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggStructConsumeGrandField(_ x: AggStruct) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.pair.lhs)
     for _ in 0..<1024 {
@@ -1254,8 +1254,8 @@ public func aggGenericStructAccessFieldOwnedArg(_ x2: __owned AggGenericStruct<K
     }
 }
 
-public func aggGenericStructConsumeField(_ x: AggGenericStruct<Klass>) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggGenericStructConsumeField(_ x: AggGenericStruct<Klass>) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.lhs)
     for _ in 0..<1024 {
@@ -1301,8 +1301,8 @@ public func aggGenericStructAccessGrandFieldOwnedArg(_ x2: __owned AggGenericStr
     }
 }
 
-public func aggGenericStructConsumeGrandField(_ x: AggGenericStruct<Klass>) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggGenericStructConsumeGrandField(_ x: AggGenericStruct<Klass>) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.pair.lhs)
     for _ in 0..<1024 {
@@ -1522,8 +1522,8 @@ public func aggGenericStructAccessFieldOwnedArg<T>(_ x2: __owned AggGenericStruc
     }
 }
 
-public func aggGenericStructConsumeField<T>(_ x: AggGenericStruct<T>) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggGenericStructConsumeField<T>(_ x: AggGenericStruct<T>) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.lhs)
     for _ in 0..<1024 {
@@ -1569,8 +1569,8 @@ public func aggGenericStructAccessGrandFieldOwnedArg<T>(_ x2: __owned AggGeneric
     }
 }
 
-public func aggGenericStructConsumeGrandField<T>(_ x: AggGenericStruct<T>) { // expected-error {{'x' has guaranteed ownership but was consumed}}
-    let x2 = x // expected-note {{consuming use}}
+public func aggGenericStructConsumeGrandField<T>(_ x: AggGenericStruct<T>) {
+    let x2 = x
     // expected-error @-1 {{'x2' has a move only field that was consumed before later uses}}
     classConsume(x2.pair.lhs)
     for _ in 0..<1024 {