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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9069
       Builder.createNaryOp(Instruction::Select, {Cond, Red, Phi});
+      NumRegularReductions++;
     }
----------------
Ayal wrote:
> #ifndef NDEBUG?
> 
> perhaps an overkill - would CM.getInLoopReductionChains() eventually be discarded?
Yes I think `CM.getInLoopReductionChains` is going away at some point. But until then it might be good to try to ensure they do not diverge. But I'd be more than happy to drop it as well, it might just be a bit too paranoid


================
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:
> > > 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?
> Agreed - the overhead of checking the cycle/chain to figure out if the reduction is in-loop or not, should be tolerable. It's about having recipes clearly model the code they represent up-front, during VPlan construction and/or planning, than whether to cache the results of this "isInLoop" query eagerly versus (re)running it lazily on demand during VPlan execution. A phi recipe representing an in-loop reduction is arguably conceptually distinct from a "regular" VPWidenPHIRecipe, with respect to the scalar-versus-vector data each operates on, and the epilogue each feeds. If the "cached result" needs to be updated, so may the cost of the recipe and of dependent recipes. Sounds reasonable?
I went ahead and added a `VPWidenPHIRecipe::isInLoopReduction` helper. 

I'm not 100% sure this is  in line with your suggestions, please let me know if I misunderstood and we should add a flag as well to start with :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102