Merge pull request #63337 from gottesmm/pr-d2869554cff24b4059130c3f6192a1417ed9b9cb

[move-only] When performing borrow to destructure transform and failing, make sure we clean up appropriately.

Author: gottesmm

lib/SILOptimizer/Mandatory/CMakeLists.txt

@@ -26,6 +26,7 @@ target_sources(swiftSILOptimizer PRIVATE
   MovedAsyncVarDebugInfoPropagator.cpp
   MoveOnlyAddressChecker.cpp
   MoveOnlyBorrowToDestructureTransform.cpp
+  MoveOnlyBorrowToDestructureTransformTester.cpp
   MoveOnlyDeinitInsertion.cpp
   MoveOnlyDiagnostics.cpp
   MoveOnlyObjectChecker.cpp

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

@@ -10,11 +10,16 @@
 //
 //===----------------------------------------------------------------------===//
 ///
-/// \file This is a pass that converts the borrow + gep pattern to destructures
-/// or emits an error if it cannot be done. It is assumed that it runs
-/// immediately before move checking of objects runs. This ensures that the move
-/// checker does not need to worry about this problem and instead can just check
-/// that the newly inserted destructures do not cause move only errors.
+/// \file This is a transform that converts the borrow + gep pattern to
+/// destructures or emits an error if it cannot be done. It is assumed that it
+/// runs immediately before move checking of objects runs. This ensures that the
+/// move checker does not need to worry about this problem and instead can just
+/// check that the newly inserted destructures do not cause move only errors.
+///
+/// This is written as a utility so that we can have a utility pass that tests
+/// this directly but also invoke this via the move only object checker.
+///
+/// TODO: Move this to SILOptimizer/Utils.
 ///
 //===----------------------------------------------------------------------===//
 
@@ -28,6 +33,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 +44,6 @@
 using namespace swift;
 using namespace swift::siloptimizer;
 
-using namespace swift;
-using namespace swift::siloptimizer;
-
 namespace {
 using AvailableValues = BorrowToDestructureTransform::AvailableValues;
 }
@@ -202,8 +206,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 +221,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");
@@ -1323,115 +1327,3 @@ void BorrowToDestructureTransform::cleanup(
   // And finally do the same thing for our initial copy_value.
   addCompensatingDestroys(liveness, boundary, initialValue);
 }
-
-//===----------------------------------------------------------------------===//
-//                            Top Level Entrypoint
-//===----------------------------------------------------------------------===//
-
-static bool runTransform(SILFunction *fn,
-                         ArrayRef<MarkMustCheckInst *> moveIntroducersToProcess,
-                         PostOrderAnalysis *poa,
-                         DiagnosticEmitter &diagnosticEmitter) {
-  BorrowToDestructureTransform::IntervalMapAllocator allocator;
-  bool madeChange = false;
-  while (!moveIntroducersToProcess.empty()) {
-    auto *mmci = moveIntroducersToProcess.back();
-    moveIntroducersToProcess = moveIntroducersToProcess.drop_back();
-
-    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;
-
-    // If we do not have any borrows to process, continue and process the next
-    // instruction.
-    if (borrowWorklist.empty())
-      continue;
-
-    SmallVector<SILBasicBlock *, 8> discoveredBlocks;
-
-    // Now that we have found all of our borrows, we want to find struct_extract
-    // uses of our borrow as well as any operands that cannot use an owned
-    // value.
-    SWIFT_DEFER { discoveredBlocks.clear(); };
-    BorrowToDestructureTransform transform(allocator, mmci, diagnosticEmitter,
-                                           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;
-
-    // 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;
-
-    // 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.
-    transform.cleanup(borrowWorklist);
-  }
-
-  return madeChange;
-}
-
-namespace {
-
-class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
-  void run() override {
-    auto *fn = getFunction();
-
-    // Only run this pass if the move only language feature is enabled.
-    if (!fn->getASTContext().LangOpts.Features.contains(Feature::MoveOnly))
-      return;
-
-    // Don't rerun diagnostics on deserialized functions.
-    if (getFunction()->wasDeserializedCanonical())
-      return;
-
-    assert(fn->getModule().getStage() == SILStage::Raw &&
-           "Should only run on Raw SIL");
-
-    LLVM_DEBUG(llvm::dbgs() << "===> MoveOnly Object Checker. Visiting: "
-                            << fn->getName() << '\n');
-
-    auto *postOrderAnalysis = getAnalysis<PostOrderAnalysis>();
-
-    SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
-    DiagnosticEmitter emitter;
-
-    bool madeChange = searchForCandidateObjectMarkMustChecks(
-        getFunction(), moveIntroducersToProcess, emitter);
-    if (madeChange) {
-      invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
-    }
-
-    if (emitter.emittedAnyDiagnostics())
-      return;
-
-    auto introducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
-                                          moveIntroducersToProcess.end());
-    if (runTransform(fn, introducers, postOrderAnalysis, emitter)) {
-      invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
-    }
-  }
-};
-
-} // namespace
-
-SILTransform *swift::createMoveOnlyBorrowToDestructureTransform() {
-  return new MoveOnlyBorrowToDestructureTransformPass();
-}

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransformTester.cpp

@@ -0,0 +1,183 @@
+//===--- MoveOnlyBorrowToDestructureTransform.cpp -------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2022 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file This is a pass that converts the borrow + gep pattern to destructures
+/// or emits an error if it cannot be done. It is assumed that it runs
+/// immediately before move checking of objects runs. This ensures that the move
+/// checker does not need to worry about this problem and instead can just check
+/// that the newly inserted destructures do not cause move only errors.
+///
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "sil-move-only-checker"
+
+#include "MoveOnlyDiagnostics.h"
+#include "MoveOnlyObjectChecker.h"
+
+#include "swift/Basic/Defer.h"
+#include "swift/SIL/SILBuilder.h"
+#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"
+#include "swift/SILOptimizer/Utils/CFGOptUtils.h"
+#include "llvm/ADT/ArrayRef.h"
+
+using namespace swift;
+using namespace swift::siloptimizer;
+
+//===----------------------------------------------------------------------===//
+//                            Top Level Entrypoint
+//===----------------------------------------------------------------------===//
+
+static bool runTransform(SILFunction *fn,
+                         ArrayRef<MarkMustCheckInst *> moveIntroducersToProcess,
+                         PostOrderAnalysis *poa,
+                         DiagnosticEmitter &diagnosticEmitter) {
+  BorrowToDestructureTransform::IntervalMapAllocator allocator;
+  bool madeChange = false;
+  while (!moveIntroducersToProcess.empty()) {
+    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, 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.
+    if (borrowWorklist.empty())
+      continue;
+
+    SmallVector<SILBasicBlock *, 8> discoveredBlocks;
+
+    // Now that we have found all of our borrows, we want to find struct_extract
+    // uses of our borrow as well as any operands that cannot use an owned
+    // value.
+    SWIFT_DEFER { discoveredBlocks.clear(); };
+    BorrowToDestructureTransform transform(allocator, mmci, diagnosticEmitter,
+                                           poa, discoveredBlocks);
+
+    // Attempt to gather uses. Return if we saw something that we did not
+    // 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, 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;
+}
+
+namespace {
+
+class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
+  void run() override {
+    auto *fn = getFunction();
+
+    // Only run this pass if the move only language feature is enabled.
+    if (!fn->getASTContext().LangOpts.Features.contains(Feature::MoveOnly))
+      return;
+
+    // Don't rerun diagnostics on deserialized functions.
+    if (getFunction()->wasDeserializedCanonical())
+      return;
+
+    assert(fn->getModule().getStage() == SILStage::Raw &&
+           "Should only run on Raw SIL");
+
+    LLVM_DEBUG(llvm::dbgs()
+               << "===> MoveOnlyBorrowToDestructureTransform. Visiting: "
+               << fn->getName() << '\n');
+
+    auto *postOrderAnalysis = getAnalysis<PostOrderAnalysis>();
+
+    SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
+    DiagnosticEmitter emitter;
+
+    bool madeChange = searchForCandidateObjectMarkMustChecks(
+        getFunction(), moveIntroducersToProcess, emitter);
+    if (madeChange) {
+      invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
+    }
+
+    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);
+    }
+  }
+};
+
+} // namespace
+
+SILTransform *swift::createMoveOnlyBorrowToDestructureTransform() {
+  return new MoveOnlyBorrowToDestructureTransformPass();
+}

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: