[move-only] Refactor CanonicalizeOSSALifetime::canonicalizeValueLifetime into an API that computes liveness and a second API that rewrites copies/destroys and fix up MoveOnly checkers to use it.

For those who are unaware, CanonicalizeOSSALifetime::canonicalizeValueLifetime()
is really a high level driver routine for the functionality of
CanonicalizeOSSALifetime that computes liveness and then rewrites copies using
boundary information. This change introduces splits the implementation of
canonicalizeValueLifetime into two parts: a first part called computeLiveness
and a second part called rewriteLifetimes. Internally canonicalizeValueLifetime
still just calls these two methods.

The reason why I am doing this is that it lets the move only object checker use
the raw liveness information computed before the rewriting mucks with the
analysis information. This information is used by the checker to compute the raw
liveness boundary of a value and use that information to determine the list of
consuming uses not on the boundary, consuming uses on the boundary, and
non-consuming uses on the boundary. This is then used by later parts of the
checker to emit our errors.

Some additional benefits of doing this are:

1. I was able to eliminate callbacks in the rewriting stage of
CanonicalOSSALifetimes which previously gave the checker this information.

2. Previously the move checker did not have access to the non-consuming boundary
uses causing us to always fail appropriately, but sadly not emit a note showing
the non-consuming use. I am going to wire this up in a subsequent commit.

The other change to the implementation of the move checker that this caused is
that I needed to add an extra diagnostic check for instructions that consume the
value twice or consume the value and use the value. The reason why this must be
done is that liveness does not distinguish in between different operands on the
same instruction meaning such an error would be lost.

Author: gottesmm

include/swift/SIL/PrunedLiveness.h

@@ -132,8 +132,10 @@
 #include "swift/SIL/OwnershipUtils.h"
 #include "swift/SIL/SILBasicBlock.h"
 #include "swift/SIL/SILFunction.h"
+#include "swift/SIL/SILInstruction.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 
 namespace swift {
@@ -542,6 +544,47 @@ class PrunedLiveness {
     return useIter->second ? LifetimeEndingUse : NonLifetimeEndingUse;
   }
 
+  using ConstUserRange =
+      iterator_range<const std::pair<SILInstruction *, bool> *>;
+  ConstUserRange getAllUsers() const {
+    return llvm::make_range(users.begin(), users.end());
+  }
+
+  /// A namespace containing helper functors for use with various mapped
+  /// ranges. Intended to be used to hide these noise types when working in an
+  /// IDE.
+  struct LifetimeEndingUserIteratorHelpers {
+    struct MapFunctor {
+      SILInstruction *
+      operator()(const std::pair<SILInstruction *, bool> &pair) const {
+        // Strip off the const to ease use with other APIs.
+        return const_cast<SILInstruction *>(pair.first);
+      }
+    };
+
+    struct FilterFunctor {
+      bool operator()(const std::pair<SILInstruction *, bool> &pair) const {
+        return pair.second;
+      }
+    };
+
+    using MapFilterIter = llvm::mapped_iterator<
+        llvm::filter_iterator<const std::pair<SILInstruction *, bool> *,
+                              FilterFunctor>,
+        MapFunctor>;
+  };
+  using LifetimeEndingUserRange =
+      llvm::iterator_range<LifetimeEndingUserIteratorHelpers::MapFilterIter>;
+
+  /// Return a range consisting of the current set of consuming users fed into
+  /// this PrunedLiveness instance.
+  LifetimeEndingUserRange getLifetimeEndingUsers() const {
+    return map_range(
+        llvm::make_filter_range(
+            getAllUsers(), LifetimeEndingUserIteratorHelpers::FilterFunctor()),
+        LifetimeEndingUserIteratorHelpers::MapFunctor());
+  }
+
   void print(llvm::raw_ostream &OS) const;
   void dump() const;
 };

include/swift/SILOptimizer/Utils/CanonicalizeOSSALifetime.h

@@ -213,15 +213,6 @@ class CanonicalizeOSSALifetime final {
   /// copies.
   bool maximizeLifetime;
 
-  /// If true and we are processing a value of move_only type, emit a diagnostic
-  /// when-ever we need to insert a copy_value.
-  std::function<void(Operand *)> moveOnlyCopyValueNotification;
-
-  /// If true and we are processing a value of move_only type, pass back to the
-  /// caller any consuming uses that are going to be used as part of the final
-  /// lifetime boundary in case we need to emit diagnostics.
-  std::function<void(Operand *)> moveOnlyFinalConsumingUse;
-
   // If present, will be used to ensure that the lifetime is not shortened to
   // end inside an access scope which it previously enclosed.  (Note that ending
   // before such an access scope is fine regardless.)
@@ -289,27 +280,10 @@ class CanonicalizeOSSALifetime final {
     }
   }
 
-  void maybeNotifyMoveOnlyCopy(Operand *use) {
-    if (!moveOnlyCopyValueNotification)
-      return;
-    moveOnlyCopyValueNotification(use);
-  }
-
-  void maybeNotifyFinalConsumingUse(Operand *use) {
-    if (!moveOnlyFinalConsumingUse)
-      return;
-    moveOnlyFinalConsumingUse(use);
-  }
-
-  CanonicalizeOSSALifetime(
-      bool pruneDebugMode, bool maximizeLifetime,
-      NonLocalAccessBlockAnalysis *accessBlockAnalysis, DominanceInfo *domTree,
-      InstructionDeleter &deleter,
-      std::function<void(Operand *)> moveOnlyCopyValueNotification = nullptr,
-      std::function<void(Operand *)> moveOnlyFinalConsumingUse = nullptr)
+  CanonicalizeOSSALifetime(bool pruneDebugMode, bool maximizeLifetime,
+                           NonLocalAccessBlockAnalysis *accessBlockAnalysis,
+                           DominanceInfo *domTree, InstructionDeleter &deleter)
       : pruneDebugMode(pruneDebugMode), maximizeLifetime(maximizeLifetime),
-        moveOnlyCopyValueNotification(moveOnlyCopyValueNotification),
-        moveOnlyFinalConsumingUse(moveOnlyFinalConsumingUse),
         accessBlockAnalysis(accessBlockAnalysis), domTree(domTree),
         deleter(deleter),
         liveness(maximizeLifetime ? &discoveredBlocks : nullptr) {}
@@ -348,8 +322,47 @@ class CanonicalizeOSSALifetime final {
   /// operands.
   bool canonicalizeValueLifetime(SILValue def);
 
+  /// Compute the liveness information for \p def. But do not do any rewriting
+  /// or computation of boundaries.
+  ///
+  /// The intention is that this is used if one wants to emit diagnostics using
+  /// the liveness information before doing any rewriting.
+  bool computeLiveness(SILValue def);
+
+  /// Given the already computed liveness boundary for the given def, rewrite
+  /// copies of def as appropriate.
+  ///
+  /// NOTE: It is assumed that one passes the extended boundary from \see
+  /// computeLiveness.
+  ///
+  /// NOTE: It is assumed that one has emitted any diagnostics.
+  void rewriteLifetimes();
+
+  /// Return the pure original boundary just based off of liveness information
+  /// without maximizing or extending liveness.
+  void findOriginalBoundary(PrunedLivenessBoundary &resultingOriginalBoundary);
+
   InstModCallbacks &getCallbacks() { return deleter.getCallbacks(); }
 
+  using IsInterestingUser = PrunedLiveness::IsInterestingUser;
+
+  /// Helper method that returns the isInterestingUser status of \p user in the
+  /// passed in Liveness.
+  ///
+  /// NOTE: Only call this after calling computeLivenessBoundary or the results
+  /// will not be initialized.
+  IsInterestingUser isInterestingUser(SILInstruction *user) const {
+    return liveness.isInterestingUser(user);
+  }
+
+  using LifetimeEndingUserRange = PrunedLiveness::LifetimeEndingUserRange;
+  LifetimeEndingUserRange getLifetimeEndingUsers() const {
+    return liveness.getLifetimeEndingUsers();
+  }
+
+  using UserRange = PrunedLiveness::ConstUserRange;
+  UserRange getUsers() const { return liveness.getAllUsers(); }
+
 private:
   void recordDebugValue(DebugValueInst *dvi) { debugValues.insert(dvi); }
 
@@ -362,8 +375,6 @@ class CanonicalizeOSSALifetime final {
 
   void extendLivenessThroughOverlappingAccess();
 
-  void findOriginalBoundary(PrunedLivenessBoundary &boundary);
-
   void findExtendedBoundary(PrunedLivenessBoundary const &originalBoundary,
                             PrunedLivenessBoundary &boundary);
 

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

@@ -1012,6 +1012,8 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
   // For convenience, grab the user of op.
   auto *user = op->getUser();
 
+  LLVM_DEBUG(llvm::dbgs() << "Visiting user: " << *user);
+
   // First check if we have init/reinit. These are quick/simple.
   if (::memInstMustInitialize(op)) {
     LLVM_DEBUG(llvm::dbgs() << "Found init: " << *user);

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

@@ -294,11 +294,11 @@ void BorrowToDestructureTransform::checkForErrorsOnSameInstruction() {
           continue;
 
         if (badOperand->isConsuming())
-          diagnosticEmitter.emitObjectConsumesDestructuredValueTwice(
-              mmci, use, badOperand);
+          diagnosticEmitter.emitObjectInstConsumesValueTwice(mmci, use,
+                                                             badOperand);
         else
-          diagnosticEmitter.emitObjectConsumesAndUsesDestructuredValue(
-              mmci, use, badOperand);
+          diagnosticEmitter.emitObjectInstConsumesAndUsesValue(mmci, use,
+                                                               badOperand);
         emittedError = true;
       }
 
@@ -359,7 +359,9 @@ bool BorrowToDestructureTransform::gatherBorrows(
     return true;
   }
 
-  LLVM_DEBUG(llvm::dbgs() << "Searching for borrows for inst: " << *mmci);
+  LLVM_DEBUG(llvm::dbgs() << "Performing BorrowToDestructureTramsform!\n"
+                             "Searching for borrows for inst: "
+                          << *mmci);
 
   StackList<Operand *> worklist(mmci->getFunction());
   for (auto *op : mmci->getUses())

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -153,41 +153,38 @@ void DiagnosticEmitter::emitObjectDiagnosticsForFoundUses(
     bool ignorePartialApplyUses) const {
   auto &astContext = fn->getASTContext();
 
-  for (auto *consumingUse : getCanonicalizer().consumingUsesNeedingCopy) {
+  for (auto *consumingUser : getCanonicalizer().consumingUsesNeedingCopy) {
     // See if the consuming use is an owned moveonly_to_copyable whose only
     // user is a return. In that case, use the return loc instead. We do this
     // b/c it is illegal to put a return value location on a non-return value
     // instruction... so we have to hack around this slightly.
-    auto *user = consumingUse->getUser();
-    auto loc = user->getLoc();
-    if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
+    auto loc = consumingUser->getLoc();
+    if (auto *mtc =
+            dyn_cast<MoveOnlyWrapperToCopyableValueInst>(consumingUser)) {
       if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
         loc = ri->getLoc();
       }
     }
 
-    if (ignorePartialApplyUses &&
-        isa<PartialApplyInst>(consumingUse->getUser()))
+    if (ignorePartialApplyUses && isa<PartialApplyInst>(consumingUser))
       continue;
     diagnose(astContext, loc.getSourceLoc(),
              diag::sil_moveonlychecker_consuming_use_here);
   }
 
-  for (auto *consumingUse : getCanonicalizer().finalConsumingUses) {
+  for (auto *user : getCanonicalizer().consumingBoundaryUsers) {
     // See if the consuming use is an owned moveonly_to_copyable whose only
     // user is a return. In that case, use the return loc instead. We do this
     // b/c it is illegal to put a return value location on a non-return value
     // instruction... so we have to hack around this slightly.
-    auto *user = consumingUse->getUser();
     auto loc = user->getLoc();
     if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
       if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
         loc = ri->getLoc();
       }
     }
 
-    if (ignorePartialApplyUses &&
-        isa<PartialApplyInst>(consumingUse->getUser()))
+    if (ignorePartialApplyUses && isa<PartialApplyInst>(user))
       continue;
 
     diagnose(astContext, loc.getSourceLoc(),
@@ -198,39 +195,37 @@ void DiagnosticEmitter::emitObjectDiagnosticsForFoundUses(
 void DiagnosticEmitter::emitObjectDiagnosticsForPartialApplyUses() const {
   auto &astContext = fn->getASTContext();
 
-  for (auto *consumingUse : getCanonicalizer().consumingUsesNeedingCopy) {
+  for (auto *user : getCanonicalizer().consumingUsesNeedingCopy) {
     // See if the consuming use is an owned moveonly_to_copyable whose only
     // user is a return. In that case, use the return loc instead. We do this
     // b/c it is illegal to put a return value location on a non-return value
     // instruction... so we have to hack around this slightly.
-    auto *user = consumingUse->getUser();
     auto loc = user->getLoc();
     if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
       if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
         loc = ri->getLoc();
       }
     }
 
-    if (!isa<PartialApplyInst>(consumingUse->getUser()))
+    if (!isa<PartialApplyInst>(user))
       continue;
     diagnose(astContext, loc.getSourceLoc(),
              diag::sil_moveonlychecker_consuming_closure_use_here);
   }
 
-  for (auto *consumingUse : getCanonicalizer().finalConsumingUses) {
+  for (auto *user : getCanonicalizer().consumingBoundaryUsers) {
     // See if the consuming use is an owned moveonly_to_copyable whose only
     // user is a return. In that case, use the return loc instead. We do this
     // b/c it is illegal to put a return value location on a non-return value
     // instruction... so we have to hack around this slightly.
-    auto *user = consumingUse->getUser();
     auto loc = user->getLoc();
     if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
       if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
         loc = ri->getLoc();
       }
     }
 
-    if (!isa<PartialApplyInst>(consumingUse->getUser()))
+    if (!isa<PartialApplyInst>(user))
       continue;
 
     diagnose(astContext, loc.getSourceLoc(),
@@ -454,14 +449,13 @@ void DiagnosticEmitter::emitObjectDestructureNeededWithinBorrowBoundary(
   registerDiagnosticEmitted(markedValue);
 }
 
-void DiagnosticEmitter::emitObjectConsumesDestructuredValueTwice(
+void DiagnosticEmitter::emitObjectInstConsumesValueTwice(
     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() << "Emitting object consumes value twice error!\n");
   LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
   LLVM_DEBUG(llvm::dbgs() << "    User: " << *firstUse->getUser());
   LLVM_DEBUG(llvm::dbgs() << "    First Conflicting Operand: "
@@ -480,15 +474,14 @@ void DiagnosticEmitter::emitObjectConsumesDestructuredValueTwice(
   registerDiagnosticEmitted(markedValue);
 }
 
-void DiagnosticEmitter::emitObjectConsumesAndUsesDestructuredValue(
+void DiagnosticEmitter::emitObjectInstConsumesAndUsesValue(
     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() << "Emitting object consumeed and used error!\n");
   LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
   LLVM_DEBUG(llvm::dbgs() << "    User: " << *consumingUse->getUser());
   LLVM_DEBUG(llvm::dbgs() << "    Consuming Operand: "

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

@@ -91,13 +91,12 @@ class DiagnosticEmitter {
       TypeTreeLeafTypeRange destructureNeededBits,
       FieldSensitivePrunedLivenessBoundary &boundary);
 
-  void emitObjectConsumesDestructuredValueTwice(MarkMustCheckInst *markedValue,
-                                                Operand *firstConsumingUse,
-                                                Operand *secondConsumingUse);
-  void
-  emitObjectConsumesAndUsesDestructuredValue(MarkMustCheckInst *markedValue,
-                                             Operand *consumingUse,
-                                             Operand *nonConsumingUse);
+  void emitObjectInstConsumesValueTwice(MarkMustCheckInst *markedValue,
+                                        Operand *firstConsumingUse,
+                                        Operand *secondConsumingUse);
+  void emitObjectInstConsumesAndUsesValue(MarkMustCheckInst *markedValue,
+                                          Operand *consumingUse,
+                                          Operand *nonConsumingUse);
 
 private:
   /// Emit diagnostics for the final consuming uses and consuming uses needing