[PATCH] D99294: [VPlan] Representing backedge def-use feeding reduction phis.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4299
   // Reductions do not have to start at zero. They can start with
   // any loop invariant values.
   for (unsigned Part = 0; Part < UF; ++Part) {
----------------
Ayal wrote:
> nit: worth hoisting `LoopVectorLatch = LI->getLoopFor(LoopVectorBody)->getLoopLatch()` here.
> (admittedly independent of this patch)
I'll do that in a separate commit.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4301
   for (unsigned Part = 0; Part < UF; ++Part) {
-    Value *VecRdxPhi = State.get(PhiR->getVPValue(), Part);
-    Value *Val = State.get(State.Plan->getVPValue(LoopVal), Part);
+    Value *VecRdxPhi = State.get(PhiR->getVPValue(0), Part);
+    Value *Val = State.get(PhiR->getOperand(1), Part);
----------------
Ayal wrote:
> nit: can continue to obtain the first-and-only VPValue defined by PhiR, by calling getVPValue() as before, which defaults to getVPValue(0), right?
> 
> 
Yep, changed it back.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8864
   }
+
+  SmallVector<PHINode *, 4> PhisToFix;
----------------
Ayal wrote:
> Worth a comment here explaining that source and destination of cross-iteration dependences are marked here in order to wire their recipes in VPlan later.
> 
> Current implementation looks fine. Raising a possible alternative, if preferred?
> The wiring of header phi's could be delegated to RecipeBuilder: have it collect the 'partially constructed' widenPhiRecipes of reductions as it creates them, into a local "PhiRecipesToFix", along with recording the recipes of their latched-operands (only). Then have RecipeBuilder::fixHeaderPhi's() to complete the construction of its PhiRecipesToFix, via their underlying Phi's.
Thanks for the suggestion. Doing the tracking and fixup directly in VPRecipeBuilder seems preferable. I updated the patch and added the additional comments to the new code.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:991
+/// descriptor, the start value is the first operand of the recipe and the
+/// incoming value from the backedge is the second operand. In the VPlan native
+/// path, all incoming VPValues & VPBasicBlock pairs are managed in the recipe
----------------
Ayal wrote:
> Worth supplying another interface to retrieve 'preheadered' and 'latched' operands of a header phi recipe (by setting them as 0 and 1 enum values?) to increase the readability of 'getOperand(0)' and 'getOperand(1)'?
I added a `VPWidenPHIRecipe::getBackedgeValue()`, similar to `getStartValue`. WDYT?


================
Comment at: llvm/test/Transforms/LoopVectorize/vplan-printing.ll:82
 ; CHECK-NEXT:   WIDEN-INDUCTION %iv = phi %iv.next, 0
-; CHECK-NEXT:   WIDEN-PHI %red = phi %red.next, 0.000000e+00
+; CHECK-NEXT:   WIDEN-PHI ir<%red> = phi ir<0.000000e+00>, ir<%red.next>
 ; CHECK-NEXT:   CLONE ir<%arrayidx> = getelementptr ir<%y>, ir<%iv>
----------------
Ayal wrote:
> Clean up the TODO in VPlan.cpp/ VPWidenPHIRecipe::print() ?
Unfortunately there are still cases where we do not track the value from the back-edge (like first-order recurrences).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99294