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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9061
+
+      if (!Cond) {
+        Builder.setInsertPoint(VPBB);
----------------
Ayal wrote:
> This is to avoid redundant creation of the header block in-mask when the loop contains no non-in-loop reductions?
> Worth doing in a separate patch?
There's a subtle change here: BlockInMask may currently be created redundantly here if there are reductions but no in-loop reductions; this modification eliminates this redundancy.
Probably insignificant, given foldTailByMasking?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9069
       Builder.createNaryOp(Instruction::Select, {Cond, Red, Phi});
+      NumRegularReductions++;
     }
----------------
fhahn wrote:
> 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
I'd vote for cleaner, paranoia free code in this case.

One could verify the integrity of in-loop reduction chains (or VPlan in general?), say before code-gen; i.e., that in-loop header phi recipes are consistent with reduction recipes.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9177
+      if (!PhiR || !PhiR->getRecurrenceDescriptor() ||
+          PhiR->isInLoopReduction())
         continue;
----------------
nit: suffice to check `if (!PhiR || PhiR->isInLoopReduction())` - the latter implies PhiR has a recurrence descriptor.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9257
   for (auto &Reduction : CM.getInLoopReductionChains()) {
     PHINode *Phi = Reduction.first;
     RecurrenceDescriptor &RdxDesc = Legal->getReductionVars()[Phi];
----------------
Wouldn't it be better, if in addition to converting widen recipes of in-loop-reduction chains into reduction recipes here, we also convert their header phi upfront:
```
    auto *PhiR = cast<VPWidenPHIRecipe>(RecipeBuilder.getRecipe(Phi));
    PhiR->setIsInLoopReduction();

```
?

(Making sure to record the recipe of Phi)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4282
+  bool IsInLoopReductionPhi = PhiR->getNumOperands() == 2 &&
+                              isa<VPReductionRecipe>(PhiR->getOperand(1));
 
----------------
fhahn wrote:
> 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 :)
This API looks good to me, thanks!
Adding a flag to implement it, as argued above, would also be good.
As stated elsewhere, there are actually 3 types of reductions, including two types of in-loop reductions: ordered and unordered.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1076
 
+bool VPWidenPHIRecipe::isInLoopReduction() const {
+  if (!RdxDesc)
----------------
(being 'const' it can't even cache the result for repeated queries...)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102