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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 6 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8979
   adjustRecipesForReductions(cast<VPBasicBlock>(TopRegion->getExiting()), Plan,
                              RecipeBuilder, Range.Start);
 
----------------
Ayal wrote:
> (Independent of this patch) Can/Should adjustRecipesForReductions() be moved to VPlanTransforms?
Unfortunately the function still uses the cost model in multiple places; some should be relatively straight-forward to replace as a first step though.


================
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:
> 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.
Done, also added a comment to VPlanTransforms.h


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:15
 #include "VPlanTransforms.h"
+#include "VPRecipeBuilder.h"
 #include "VPlanCFG.h"
----------------
Ayal wrote:
> 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).
Yes, I think that was why clang-format moved it out-of-place. Will fix separately.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589