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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks! Adding couple of final nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8979
   adjustRecipesForReductions(cast<VPBasicBlock>(TopRegion->getExiting()), Plan,
                              RecipeBuilder, Range.Start);
 
----------------
(Independent of this patch) Can/Should adjustRecipesForReductions() be moved to VPlanTransforms?


================
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:
> fhahn wrote:
> > Ayal wrote:
> > > nit: FOR now stands for FixedOrderRecurrence which leads to fixFixedOrderRecurrences() ... how about addSplicesToFixedOrderRecurrences()?
> > Updated to `adjustFixedOrderRecurrences`, as it also requires sinking. WDYT?
> Very good. A comment may help explain what `adjust` stands for.
Worth documenting what adjustFixedOrderRecurrences() does.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:589
+  Seen.insert(Previous);
+  auto TryToPushSinkCandidate = [&](VPRecipeBase *SinkCandidate) {
+    if (isa<VPHeaderPHIRecipe>(SinkCandidate) ||
----------------
Worth asserting SinkCandidate != Previous? FORs shouldn't close a dependence cycle.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:607
+  // processed first.
+  sort(RecipesToSink, [&VPDT](const VPRecipeBase *A, const VPRecipeBase *B) {
+    auto *RegionA = GetReplicateRegion(const_cast<VPRecipeBase *>(A));
----------------
fhahn wrote:
> Ayal wrote:
> > Recipes to sink are already held in a set: can `Seen` be set to use this order and serve as RecipesToSink?
> SmallPtrSet and SetVector cannot be sorted unfortunately. Updated to use WorkList.
Thought was to use some other set with "compare" set to dominance order (std::set? Admittedly not SmallPtrSet) to indicate which candidates were already seen and to later traverse them in the desired order.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:15
 #include "VPlanTransforms.h"
+#include "VPRecipeBuilder.h"
 #include "VPlanCFG.h"
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > nit: lex order?
> > > moved, I am not sure why clang-format didn't fix it
> > still one out of place?
> argh yes, fixed!
ah, includes still seem out of lex order (they're not quite in order w/o the patch).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:81
+
+  static void adjustFixedOrderRecurrences(VPlan &Plan, VPBuilder &Builder);
 };
----------------
Better keep optimizeForVFAndUF() last, as it operates after BestVP and BestUF have been chosen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589