Merge pull request #63342 from gottesmm/pr-be1c0e09106b26c3a14caa3e66d52a889dc36491

[move-only] Teach the borrow to destructure transform how to handle consuming/non-consuming uses on the same instruction.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -727,6 +727,8 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
 // move only checker diagnostics
 ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))
+ERROR(sil_moveonlychecker_owned_value_consumed_and_used_at_same_time, none,
+      "'%0' consumed and used at the same time", (StringRef))
 ERROR(sil_moveonlychecker_value_used_after_consume, none,
       "'%0' used after consume. Lifetime extension of variable requires a copy", (StringRef))
 ERROR(sil_moveonlychecker_guaranteed_value_consumed, none,
@@ -751,11 +753,15 @@ NOTE(sil_moveonlychecker_moveonly_field_consumed_here, none,
 NOTE(sil_moveonlychecker_boundary_use, none,
      "boundary use here", ())
 NOTE(sil_moveonlychecker_consuming_use_here, none,
-     "consuming use", ())
+     "consuming use here", ())
+NOTE(sil_moveonlychecker_two_consuming_uses_here, none,
+     "two consuming uses here", ())
+NOTE(sil_moveonlychecker_consuming_and_non_consuming_uses_here, none,
+     "consuming and non-consuming uses here", ())
 NOTE(sil_moveonlychecker_consuming_closure_use_here, none,
-     "closure capture", ())
+     "closure capture here", ())
 NOTE(sil_moveonlychecker_nonconsuming_use_here, none,
-     "non-consuming use", ())
+     "non-consuming use here", ())
 ERROR(sil_moveonlychecker_not_understand_no_implicit_copy, none,
       "Usage of @noImplicitCopy that the move checker does not know how to "
       "check!", ())

include/swift/AST/DiagnosticsSema.def

@@ -6653,6 +6653,8 @@ ERROR(noimplicitcopy_attr_valid_only_on_local_let_params,
       none, "'@_noImplicitCopy' attribute can only be applied to local lets and params", ())
 ERROR(noimplicitcopy_attr_invalid_in_generic_context,
       none, "'@_noImplicitCopy' attribute cannot be applied to entities in generic contexts", ())
+ERROR(noimplicitcopy_attr_not_allowed_on_moveonlytype,none,
+      "'@_noImplicitCopy' has no effect when applied to a move only type", ())
 
 //------------------------------------------------------------------------------
 // MARK: Type inference from default expressions

lib/SILGen/SILGenLValue.cpp

@@ -21,6 +21,7 @@
 #include "ExecutorBreadcrumb.h"
 #include "Initialization.h"
 #include "LValue.h"
+#include "ManagedValue.h"
 #include "RValue.h"
 #include "SILGen.h"
 #include "SILGenFunction.h"
@@ -4606,6 +4607,19 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
                      substFormalType, rvalueTL, C, IsNotTake, isBaseGuaranteed);
       } else if (isReadAccessResultOwned(src.getAccessKind()) &&
           !projection.isPlusOne(*this)) {
+
+        // Before we copy, if we have a move only wrapped value, unwrap the
+        // value using a guaranteed moveonlywrapper_to_copyable.
+        if (projection.getType().isMoveOnlyWrapped()) {
+          // We are assuming we always get a guaranteed value here.
+          assert(projection.getValue()->getOwnershipKind() ==
+                 OwnershipKind::Guaranteed);
+          // We use SILValues here to ensure we get a tight scope around our
+          // copy.
+          projection =
+              B.createGuaranteedMoveOnlyWrapperToCopyableValue(loc, projection);
+        }
+
         projection = projection.copy(*this, loc);
       }
 

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

@@ -28,18 +28,20 @@
 #include "MoveOnlyDiagnostics.h"
 #include "MoveOnlyObjectChecker.h"
 
+#include "swift/Basic/BlotSetVector.h"
 #include "swift/Basic/Defer.h"
+#include "swift/Basic/FrozenMultiMap.h"
+#include "swift/SIL/FieldSensitivePrunedLiveness.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"
+#include "llvm/ADT/SmallBitVector.h"
 
 using namespace swift;
 using namespace swift::siloptimizer;
@@ -143,6 +145,7 @@ bool BorrowToDestructureTransform::gatherUses(
           {nextUse, {*leafRange, false /*is lifetime ending*/}});
       liveness.updateForUse(nextUse->getUser(), *leafRange,
                             false /*is lifetime ending*/);
+      instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
       continue;
     }
 
@@ -166,6 +169,7 @@ bool BorrowToDestructureTransform::gatherUses(
                           {nextUse, {*leafRange, true /*is lifetime ending*/}});
       liveness.updateForUse(nextUse->getUser(), *leafRange,
                             true /*is lifetime ending*/);
+      instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
       continue;
     }
 
@@ -198,39 +202,163 @@ bool BorrowToDestructureTransform::gatherUses(
   return true;
 }
 
+void BorrowToDestructureTransform::checkForErrorsOnSameInstruction() {
+  // At this point, we have emitted all boundary checks. We also now need to
+  // check if any of our consuming uses that are on the boundary are used by the
+  // same instruction as a different consuming or non-consuming use.
+  instToInterestingOperandIndexMap.setFrozen();
+  SmallBitVector usedBits(liveness.getNumSubElements());
+
+  for (auto instRangePair : instToInterestingOperandIndexMap.getRange()) {
+    SWIFT_DEFER { usedBits.reset(); };
+
+    // First loop through our uses and handle any consuming twice errors. We
+    // also setup usedBits to check for non-consuming uses that may overlap.
+    Operand *badOperand = nullptr;
+    Optional<TypeTreeLeafTypeRange> badRange;
+    for (auto *use : instRangePair.second) {
+      if (!use->isConsuming())
+        continue;
+
+      auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+      for (unsigned index : destructureUseSpan.getRange()) {
+        if (usedBits[index]) {
+          // If we get that we used the same bit twice, we have an error. We set
+          // the badIndex error and break early.
+          badOperand = use;
+          badRange = destructureUseSpan;
+          break;
+        }
+
+        usedBits[index] = true;
+      }
+
+      // If we set badOperand, break so we can emit an error for this
+      // instruction.
+      if (badOperand)
+        break;
+    }
+
+    // If we did not set badIndex for consuming uses, we did not have any
+    // conflicts among consuming uses. see if we have any conflicts with
+    // non-consuming uses. Otherwise, we continue.
+    if (!badOperand) {
+      for (auto *use : instRangePair.second) {
+        if (use->isConsuming())
+          continue;
+
+        auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+        for (unsigned index : destructureUseSpan.getRange()) {
+          if (!usedBits[index])
+            continue;
+
+          // If we get that we used the same bit twice, we have an error. We set
+          // the badIndex error and break early.
+          badOperand = use;
+          badRange = destructureUseSpan;
+          break;
+        }
+
+        // If we set badOperand, break so we can emit an error for this
+        // instruction.
+        if (badOperand)
+          break;
+      }
+
+      // If we even did not find a non-consuming use that conflicts, then
+      // continue.
+      if (!badOperand)
+        continue;
+    }
+
+    // If badIndex is set, we broke out of the inner loop and need to emit an
+    // error. Use a little more compile time to identify the other operand that
+    // caused the failure. NOTE: badOperand /could/ be a non-consuming use, but
+    // the use we are identifying here will always be consuming.
+    usedBits.reset();
+
+    // Reinitialize use bits with the bad bits.
+    for (unsigned index : badRange->getRange())
+      usedBits[index] = true;
+
+    // Now loop back through looking for the original operand that set the used
+    // bits. This will always be a consuming use.
+    for (auto *use : instRangePair.second) {
+      if (!use->isConsuming())
+        continue;
+
+      auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+      bool emittedError = false;
+      for (unsigned index : destructureUseSpan.getRange()) {
+        if (!usedBits[index])
+          continue;
+
+        if (badOperand->isConsuming())
+          diagnosticEmitter.emitObjectConsumesDestructuredValueTwice(
+              mmci, use, badOperand);
+        else
+          diagnosticEmitter.emitObjectConsumesAndUsesDestructuredValue(
+              mmci, use, badOperand);
+        emittedError = true;
+      }
+
+      // Once we have emitted the error, just break out of the loop.
+      if (emittedError)
+        break;
+    }
+  }
+}
+
 void BorrowToDestructureTransform::checkDestructureUsesOnBoundary() const {
   LLVM_DEBUG(llvm::dbgs() << "Checking destructure uses on boundary!\n");
+
   // Now that we have found all of our destructure needing uses and liveness
   // needing uses, make sure that none of our destructure needing uses are
-  // within our boundary. If so, we have an automatic error.
+  // within our boundary. If so, we have an automatic error since we have a
+  // use-after-free.
   for (auto *use : destructureNeedingUses) {
     LLVM_DEBUG(llvm::dbgs()
                << "    DestructureNeedingUse: " << *use->getUser());
+
     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.
-      //
-      // NOTE: Since we are going to emit an error here, we do the boundary
-      // computation to ensure that we only do the boundary computation once:
-      // when we emit an error or once we know we need to do rewriting.
-      //
-      // TODO: Fix diagnostic to use destructure needing use and boundary
-      // uses.
-      FieldSensitivePrunedLivenessBoundary boundary(
-          liveness.getNumSubElements());
-      liveness.computeBoundary(boundary);
-      diagnosticEmitter.emitObjectDestructureNeededWithinBorrowBoundary(
-          mmci, use->getUser(), destructureUseSpan, boundary);
-      return;
-    } else {
-      LLVM_DEBUG(llvm::dbgs() << "        On boundary! No error!\n");
+    if (!liveness.isWithinBoundary(use->getUser(), destructureUseSpan)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "        On boundary or within boundary! No error!\n");
+      continue;
     }
+
+    // Emit an error. We have a use after free.
+    //
+    // NOTE: Since we are going to emit an error here, we do the boundary
+    // computation to ensure that we only do the boundary computation once:
+    // when we emit an error or once we know we need to do rewriting.
+    //
+    // TODO: Fix diagnostic to use destructure needing use and boundary
+    // uses.
+    LLVM_DEBUG(llvm::dbgs() << "        Within boundary! Emitting error!\n");
+    FieldSensitivePrunedLivenessBoundary boundary(liveness.getNumSubElements());
+    liveness.computeBoundary(boundary);
+    diagnosticEmitter.emitObjectDestructureNeededWithinBorrowBoundary(
+        mmci, use->getUser(), destructureUseSpan, boundary);
   }
 }
 
 bool BorrowToDestructureTransform::gatherBorrows(
     MarkMustCheckInst *mmci, StackList<BeginBorrowInst *> &borrowWorklist) {
+  // If we have a no implicit copy mark_must_check, we do not run the borrow to
+  // destructure transform since:
+  //
+  // 1. If we have a move only type, we should have emitted an earlier error
+  //    saying that move only types should not be marked as no implicit copy.
+  //
+  // 2. If we do not have a move only type, then we know that all fields that we
+  //    access directly and would cause a need to destructure must be copyable,
+  //    so no transformation/error is needed.
+  if (mmci->getType().isMoveOnlyWrapped()) {
+    LLVM_DEBUG(llvm::dbgs() << "Skipping move only wrapped inst: " << *mmci);
+    return true;
+  }
+
   LLVM_DEBUG(llvm::dbgs() << "Searching for borrows for inst: " << *mmci);
 
   StackList<Operand *> worklist(mmci->getFunction());

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransformTester.cpp

@@ -106,6 +106,19 @@ static bool runTransform(SILFunction *fn,
       continue;
     }
 
+    // Then check if we had two consuming uses on the same instruction or a
+    // consuming/non-consuming use on the same isntruction.
+    transform.checkForErrorsOnSameInstruction();
+
+    // 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);

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -453,3 +453,56 @@ void DiagnosticEmitter::emitObjectDestructureNeededWithinBorrowBoundary(
   }
   registerDiagnosticEmitted(markedValue);
 }
+
+void DiagnosticEmitter::emitObjectConsumesDestructuredValueTwice(
+    MarkMustCheckInst *markedValue, Operand *firstUse, Operand *secondUse) {
+  assert(firstUse->getUser() == secondUse->getUser());
+  assert(firstUse->isConsuming());
+  assert(secondUse->isConsuming());
+
+  LLVM_DEBUG(
+      llvm::dbgs() << "Emitting object consumes destructure twice error!\n");
+  LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
+  LLVM_DEBUG(llvm::dbgs() << "    User: " << *firstUse->getUser());
+  LLVM_DEBUG(llvm::dbgs() << "    First Conflicting Operand: "
+                          << firstUse->getOperandNumber() << '\n');
+  LLVM_DEBUG(llvm::dbgs() << "    Second Conflicting Operand: "
+                          << secondUse->getOperandNumber() << '\n');
+
+  auto &astContext = markedValue->getModule().getASTContext();
+  StringRef varName = getVariableNameForValue(markedValue);
+  diagnose(astContext,
+           markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_owned_value_consumed_more_than_once,
+           varName);
+  diagnose(astContext, firstUse->getUser()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_two_consuming_uses_here);
+  registerDiagnosticEmitted(markedValue);
+}
+
+void DiagnosticEmitter::emitObjectConsumesAndUsesDestructuredValue(
+    MarkMustCheckInst *markedValue, Operand *consumingUse,
+    Operand *nonConsumingUse) {
+  assert(consumingUse->getUser() == nonConsumingUse->getUser());
+  assert(consumingUse->isConsuming());
+  assert(!nonConsumingUse->isConsuming());
+
+  LLVM_DEBUG(
+      llvm::dbgs() << "Emitting object consumes destructure twice error!\n");
+  LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
+  LLVM_DEBUG(llvm::dbgs() << "    User: " << *consumingUse->getUser());
+  LLVM_DEBUG(llvm::dbgs() << "    Consuming Operand: "
+                          << consumingUse->getOperandNumber() << '\n');
+  LLVM_DEBUG(llvm::dbgs() << "    Non Consuming Operand: "
+                          << nonConsumingUse->getOperandNumber() << '\n');
+
+  auto &astContext = markedValue->getModule().getASTContext();
+  StringRef varName = getVariableNameForValue(markedValue);
+  diagnose(astContext,
+           markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_owned_value_consumed_and_used_at_same_time,
+           varName);
+  diagnose(astContext, consumingUse->getUser()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_consuming_and_non_consuming_uses_here);
+  registerDiagnosticEmitted(markedValue);
+}

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

@@ -91,6 +91,14 @@ class DiagnosticEmitter {
       TypeTreeLeafTypeRange destructureNeededBits,
       FieldSensitivePrunedLivenessBoundary &boundary);
 
+  void emitObjectConsumesDestructuredValueTwice(MarkMustCheckInst *markedValue,
+                                                Operand *firstConsumingUse,
+                                                Operand *secondConsumingUse);
+  void
+  emitObjectConsumesAndUsesDestructuredValue(MarkMustCheckInst *markedValue,
+                                             Operand *consumingUse,
+                                             Operand *nonConsumingUse);
+
 private:
   /// Emit diagnostics for the final consuming uses and consuming uses needing
   /// copy. If filter is non-null, allow for the caller to pre-process operands

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -387,6 +387,18 @@ bool MoveOnlyChecker::convertBorrowExtractsToOwnedDestructures(
   unsigned diagnosticCount = diagnosticEmitter.getDiagnosticCount();
   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 (diagnosticCount != diagnosticEmitter.getDiagnosticCount())
+    return true;
+
+  // Then check if we had two consuming uses on the same instruction or a
+  // consuming/non-consuming use on the same isntruction.
+  transform.checkForErrorsOnSameInstruction();
+
   // 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