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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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);
 
----------------
Ayal wrote:
> nit: FOR now stands for FixedOrderRecurrence which leads to fixFixedOrderRecurrences() ... how about addSplicesToFixedOrderRecurrences()?
Updated to `adjustFixedOrderRecurrences`, as it also requires sinking. WDYT?


================
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));
----------------
Ayal wrote:
> Comment above deserves update.
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1262
 
+  VPValue *getPreviousValue() { return getOperand(1); }
+
----------------
Ayal wrote:
> redundant - aka VPHeaderPHIRecipe::getBackedgeValue()?
Ah yes, removed!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:15
 #include "VPlanTransforms.h"
+#include "VPRecipeBuilder.h"
 #include "VPlanCFG.h"
----------------
Ayal wrote:
> nit: lex order?
moved, I am not sure why clang-format didn't fix it


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:552
+
+static bool recipeComesBefore(const VPRecipeBase *A, const VPRecipeBase *B,
+                              VPDominatorTree &VPDT) {
----------------
Ayal wrote:
> 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.
Renamed! Might make sense to move it to VPDominatorTree eventually, but probably best to avoid the linear scan


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:579
+    if (RegionA && RegionB)
+      return VPDT.dominates(RegionA, RegionB);
+
----------------
Ayal wrote:
> 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. 
Adjusted! I also updated the code to not use a set but just sort the vector once after building the list.


================
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()) {
----------------
Ayal wrote:
> nit: assert with an error message that Current has at most a single value?
Added, thanks! It might simplify things if we would provide VPRecipeBase::users() as follow-up


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:622
+      if (recipeComesBefore(Target, Curr, VPDT))
+        Target = Curr;
+    }
----------------
Ayal wrote:
> 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?
This is not needed as long as we keep the recipes to sink in dominance order. This should ensure that we can just update `Previous = SinkCandidate`. When looking at the operands, an operand may temporarily not dominate SInkCandidate, as it has already been moved past previous. 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:707
+    }
+    if (Region || Previous->isPhi())
+      Builder.setInsertPoint(InsertBlock, InsertBlock->getFirstNonPhi());
----------------
Ayal wrote:
> 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?
It is checking for header phis, can adjust once the new sinking code has settled in?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589