[PATCH] D100102: [VPlan] Iterate over phi recipes to detect reductions to fix.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9416
         continue;
-      VPValue *Phi = Plan->getOrAddVPValue(Reduction.first);
-      VPValue *Red = Plan->getOrAddVPValue(Reduction.second.getLoopExitInstr());
-      Builder.createNaryOp(Instruction::Select, {Cond, Red, Phi});
+      if (!Cond)
+        Cond = RecipeBuilder.createBlockInMask(OrigLoop->getHeader(), Plan);
----------------
Ayal wrote:
> Checking if (!Cond) seems redundant; if we want to avoid creating BlockInMask for the header block here unless there is at-least one reduction (although it will probably be created anyhow, especially under foldTail), may as well call createBlockInMask() repeatedly - it provides internal caching. I.e., should be renamed more accurately getOrCreateBlockInMask().
> Checking if (!Cond) seems redundant; if we want to avoid creating BlockInMask for the header block here unless there is at-least one reduction (although it will probably be created anyhow, especially under foldTail), may as well call createBlockInMask() repeatedly - it provides internal caching. I.e., should be renamed more accurately getOrCreateBlockInMask().

Right, I just updated the code to unconditionally create the condition, relying on the caching.

> Perhaps wrap the following insert-select-for-non-in-loop-reduction-under-fold-tail along with the above, under adjustRecipesForReductions()? buildVPlanWithVPRecipes() has grown out of propotion.

Done!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9419
+      VPValue *Red = Plan->getOrAddVPValue(
+          PhiR->getRecurrenceDescriptor().getLoopExitInstr());
+      Builder.createNaryOp(Instruction::Select, {Cond, Red, PhiR});
----------------
Ayal wrote:
> `VPValue *Red = PhiR->getBackedgeValue ();`?
> 
> Would be good to note that (and remember how ;-) ILV::fixReduction() later hooks up to this select, as it seems pretty useless here.
I updated the code as suggested, but is the loop exit value always the backedge value? I added a note that the select here is only to model the def-use chain in VPlan (AFAIU)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102