[PATCH] D100102: [VPlan] Use incoming VPValue to detect in-loop reductions (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4282
+  bool IsInLoopReductionPhi = PhiR->getNumOperands() == 2 &&
+                              isa<VPReductionRecipe>(PhiR->getOperand(1));
 
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > number of operands must be 2, right?
> > > 
> > > this may be fragile, if some (cast?) vpinstruction is introduced between the VPReductionRecipe and the header phi it feeds? Since in-loop-reduction phi's require special code-gen handling, perhaps their recipes should be marked as such?
> > > number of operands must be 2, right?
> > 
> > It's not needed here, I removed it.
> > 
> > > this may be fragile, if some (cast?) vpinstruction is introduced between the VPReductionRecipe and the header phi it feeds? Since in-loop-reduction phi's require special code-gen handling, perhaps their recipes should be marked as such?
> > 
> > Agreed, but not sure if we need to fix this issue before landing the patch?
> > 
> > I added an assertion in the code below to catch the case where we fail to identify an in-loop reduction in VPlan.
> wouldn't it be simpler, clearer and more robust if adjustRecipesForInLoopReductions() would turn on an "IsInLoop" bit inside VPWidenPhiRecipe, when converting its associated VPWiden[Select]Recipes into VPReductionRecipes?
It would be simpler, but I'm not sure if it is worth adding this extra coupling by adding a new InLoopReduction field to the general VPWidenPHIRecipe that needs to be maintained at this point? We could also just traverse the whole cycle if needed I think as in practice it should be only a very small number of steps. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102