[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:9058
+      if (!PhiR || !PhiR->getRecurrenceDescriptor() ||
+          isa<VPReductionRecipe>(PhiR->getBackedgeValue()))
         continue;
----------------
ditto ;-)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9061
+
+      if (!Cond) {
+        Builder.setInsertPoint(VPBB);
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9069
       Builder.createNaryOp(Instruction::Select, {Cond, Red, Phi});
+      NumRegularReductions++;
     }
----------------
#ifndef NDEBUG?

perhaps an overkill - would CM.getInLoopReductionChains() eventually be discarded?


================
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:
> > > > 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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4693
+        (State.VF.isScalar()) || (PhiR->getNumOperands() == 2 &&
+                                  isa<VPReductionRecipe>(PhiR->getOperand(1)));
     Type *VecTy =
----------------
fhahn wrote:
> Ayal wrote:
> > ditto
> here PhiR could also be a first order recurrence, so for now I think we need to at least make sure it's a reduction.
Agreed; "ditto" referred to asking here too directly if `PhiR->isInLoopReduction()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102