[move-only] Refactor CanonicalizeOSSA so we can emit nicer non-consuming use notes and increase quality of errors.

include/swift/AST/DiagnosticsSIL.def

@@ -730,7 +730,7 @@ ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
 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))
+      "'%0' used after consume", (StringRef))
 ERROR(sil_moveonlychecker_guaranteed_value_consumed, none,
       "'%0' has guaranteed ownership but was consumed", (StringRef))
 ERROR(sil_moveonlychecker_guaranteed_value_captured_by_closure, none,
@@ -754,6 +754,8 @@ NOTE(sil_moveonlychecker_boundary_use, none,
      "boundary use here", ())
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use here", ())
+NOTE(sil_moveonlychecker_other_consuming_use_here, none,
+     "other 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,
@@ -762,6 +764,9 @@ NOTE(sil_moveonlychecker_consuming_closure_use_here, none,
      "closure capture here", ())
 NOTE(sil_moveonlychecker_nonconsuming_use_here, none,
      "non-consuming use here", ())
+NOTE(sil_movekillscopyablevalue_value_cyclic_consumed_in_loop_here, none,
+     "consuming in loop 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/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,76 @@ 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 RangeIterationHelpers {
+    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 LifetimeEnding {
+      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>;
+    };
+
+    struct NonLifetimeEnding {
+      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<
+      RangeIterationHelpers::LifetimeEnding::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(),
+            RangeIterationHelpers::LifetimeEnding::FilterFunctor()),
+        RangeIterationHelpers::MapFunctor());
+  }
+
+  using NonLifetimeEndingUserRange = llvm::iterator_range<
+      RangeIterationHelpers::NonLifetimeEnding::MapFilterIter>;
+
+  /// Return a range consisting of the current set of non lifetime ending users
+  /// fed into this PrunedLiveness instance.
+  NonLifetimeEndingUserRange getNonLifetimeEndingUsers() const {
+    return map_range(
+        llvm::make_filter_range(
+            getAllUsers(),
+            RangeIterationHelpers::NonLifetimeEnding::FilterFunctor()),
+        RangeIterationHelpers::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,52 @@ 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 NonLifetimeEndingUserRange = PrunedLiveness::NonLifetimeEndingUserRange;
+  NonLifetimeEndingUserRange getNonLifetimeEndingUsers() const {
+    return liveness.getNonLifetimeEndingUsers();
+  }
+
+  using UserRange = PrunedLiveness::ConstUserRange;
+  UserRange getUsers() const { return liveness.getAllUsers(); }
+
 private:
   void recordDebugValue(DebugValueInst *dvi) { debugValues.insert(dvi); }
 
@@ -362,8 +380,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())