[PATCH] D142589: [LV] Perform recurrence sinking directly on VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

A very nice step forward! Raises a few thoughts regarding recipes of predicated instructions, arguably independent of the patch.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8975
 
-  // Introduce a recipe to combine the incoming and previous values of a
-  // fixed-order recurrence.
-  for (VPRecipeBase &R :
-       Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
-    auto *RecurPhi = dyn_cast<VPFirstOrderRecurrencePHIRecipe>(&R);
-    if (!RecurPhi)
-      continue;
-
-    VPRecipeBase *PrevRecipe = &RecurPhi->getBackedgeRecipe();
-    // Fixed-order recurrences do not contain cycles, so this loop is guaranteed
-    // to terminate.
-    while (auto *PrevPhi =
-               dyn_cast<VPFirstOrderRecurrencePHIRecipe>(PrevRecipe))
-      PrevRecipe = &PrevPhi->getBackedgeRecipe();
-    VPBasicBlock *InsertBlock = PrevRecipe->getParent();
-    auto *Region = GetReplicateRegion(PrevRecipe);
-    if (Region)
-      InsertBlock = dyn_cast<VPBasicBlock>(Region->getSingleSuccessor());
-    if (!InsertBlock) {
-      InsertBlock = new VPBasicBlock(Region->getName() + ".succ");
-      VPBlockUtils::insertBlockAfter(InsertBlock, Region);
-    }
-    if (Region || PrevRecipe->isPhi())
-      Builder.setInsertPoint(InsertBlock, InsertBlock->getFirstNonPhi());
-    else
-      Builder.setInsertPoint(InsertBlock, std::next(PrevRecipe->getIterator()));
-
-    auto *RecurSplice = cast<VPInstruction>(
-        Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
-                             {RecurPhi, RecurPhi->getBackedgeValue()}));
-
-    RecurPhi->replaceAllUsesWith(RecurSplice);
-    // Set the first operand of RecurSplice to RecurPhi again, after replacing
-    // all users.
-    RecurSplice->setOperand(0, RecurPhi);
-  }
+  VPlanTransforms::fixFirstOrderRecurrences(*Plan, Builder);
 
----------------
nit: FOR now stands for FixedOrderRecurrence which leads to fixFixedOrderRecurrences() ... how about addSplicesToFixedOrderRecurrences()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8929
           // block after applying SinkAfter, which relies on the original
           // position of the trunc.
           assert(isa<TruncInst>(Instr));
----------------
Comment above deserves update.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1262
 
+  VPValue *getPreviousValue() { return getOperand(1); }
+
----------------
redundant - aka VPHeaderPHIRecipe::getBackedgeValue()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:15
 #include "VPlanTransforms.h"
+#include "VPRecipeBuilder.h"
 #include "VPlanCFG.h"
----------------
nit: lex order?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:552
+
+static bool recipeComesBefore(const VPRecipeBase *A, const VPRecipeBase *B,
+                              VPDominatorTree &VPDT) {
----------------
aka `dominates`?

Note for completeness: a recipe is considered to dominate itself, i.e., returns true if the same recipe is passed as both A and B.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:579
+    if (RegionA && RegionB)
+      return VPDT.dominates(RegionA, RegionB);
+
----------------
This checking of regions is needed rather than recipeComesBefore()'s checking of parenting basic blocks, because neither basic block dominates the other when they reside in replicate regions? What if only A resides inside a replicate region?
When two recipes belong to the same block, their dominance is clear. Otherwise, if any recipe resides inside a replicate region, should the exit block of the replicate region be considered instead of its parenting basic block? Thereby collapsing the region into a node, or considering a pred-inst-phi instead of its pred-inst. 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:609
+  for (unsigned I = 0; I != WorkList.size(); ++I) {
+    VPRecipeBase *Current = WorkList[I];
+    for (VPUser *User : Current->getVPSingleValue()->users()) {
----------------
nit: assert with an error message that Current has at most a single value?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:622
+      if (recipeComesBefore(Target, Curr, VPDT))
+        Target = Curr;
+    }
----------------
Clearly SinkCandidate should not be sunk past any of its operands, but this is a bit confusing: Curr dominates SinkCandidate by definition of being its operand, so if Previous=Target dominates Curr it follows that Previous dominates SinkCandidate - however such SinkCandidates are filtered from entering InstrsToSink?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:700
+    VPBasicBlock *InsertBlock = Previous->getParent();
+    auto *Region = GetReplicateRegion(Previous);
+    if (Region)
----------------
(Independent of this patch) Another way of thinking about this: if Previous resides inside a replicate region, treat its pred-inst-phi as if it were Previous? Indeed, such a predicated instruction does not dominate the latch...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:703
+      InsertBlock = dyn_cast<VPBasicBlock>(Region->getSingleSuccessor());
+    if (!InsertBlock) {
+      InsertBlock = new VPBasicBlock(Region->getName() + ".succ");
----------------
(Independent of this patch) Should this be under the `if (Region)`? I.e., Previous surely has a VPBasicBlock parent, and Region has a single successor, but the latter could potentially be a region rather than a VPBasicBlock?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:707
+    }
+    if (Region || Previous->isPhi())
+      Builder.setInsertPoint(InsertBlock, InsertBlock->getFirstNonPhi());
----------------
hmm, could Previous be a header phi (i.e., induction, given that reductions feed only themselves inside the loop, and we exhausted all FOR's), or is this checking for blend/pred-inst-phi's?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:714
+        Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
+                             {FOR, FOR->getBackedgeValue()}));
+
----------------
(Independent of this patch) Note that if FOR->getBackedgeValue() differs from Previous then it is a FOR header phi, so in any case it will dominate RecurSplice.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:27
 class PredicatedScalarEvolution;
+class VPBuilder;
 class TargetLibraryInfo;
----------------
nit: place last to try and keep lex order?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589