[PATCH] D142886: [VPlan] Switch to checking sinking legality for recurrences in VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:189
   /// the iteration before the previous, we refer to it as second-order
-  /// recurrence and so on). \p SinkAfter includes pairs of instructions where
-  /// the first will be rescheduled to appear after the second if/when the loop
-  /// is vectorized. It may be augmented with additional pairs if needed in
-  /// order to handle Phi as a first-order recurrence.
-  static bool
-  isFixedOrderRecurrence(PHINode *Phi, Loop *TheLoop,
-                         MapVector<Instruction *, Instruction *> &SinkAfter,
-                         DominatorTree *DT);
+  /// recurrence and so on).
+  static bool isFixedOrderRecurrence(PHINode *Phi, Loop *TheLoop,
----------------
Leave a comment that this optimistically assumes instructions may be reordered successfully if needed?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:967
 
-  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous) ||
-      SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
+  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous))
     return false;
----------------
(Relying here on Previous is fragile as it may have been considered for sinking.)


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:985
-  std::set<Instruction *, decltype(CompareByComesBefore)> InstrsToSink(
-      CompareByComesBefore);
-
----------------
nit: such a set can be used to store "Seen" recipes avoiding revisiting recipes as they are added to the worklist and then iterating over them in dominance order to sink them.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:651
   const VPRegionBlock *RegionB =
       GetReplicateRegion(const_cast<VPRecipeBase *>(B));
   if (RegionA)
----------------
nit: is this consideration of replicate regions is still needed here, or can be replaced by an assert?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:689
     VPRecipeBase *Current = WorkList[I];
-    assert(Current->getNumDefinedValues() == 1 &&
-           "only recipes with a single defined value expected");
-    for (VPUser *User : Current->getVPSingleValue()->users()) {
-      if (auto *R = dyn_cast<VPRecipeBase>(User))
-        TryToPushSinkCandidate(R);
+    for (VPValue *Val : Current->definedValues()) {
+      for (VPUser *User : Val->users()) {
----------------
Is this change to support multiple def'd recipes independent of the rest of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:740
+      return false;
+    SeenPrevious.insert(Previous);
 
----------------
Is `SeenPrevious` really needed, to make sure chained FORs are processed in order, or can it be removed - given that every FOR takes care of its own sinkings followed by introducing its splice hooking it up to defs and uses? Potentially sinking candidates multiple times.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:79
   /// current implementation assumes all users can be sunk after the previous
   /// value, which is enforced by earlier legality checks.
+  static bool adjustFixedOrderRecurrences(VPlan &Plan, VPBuilder &Builder);
----------------
Explain return value?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll:637
 
-; Make sure LLVM doesn't generate wrong data in SinkAfter, and causes crash in
-; loop vectorizer.
 define void @test_crash(ptr %p) {
+; CHECK-LABEL: @test_crash(
----------------
nit: update name of test, and its comment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142886/new/

https://reviews.llvm.org/D142886