[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:
> 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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4693
+        (State.VF.isScalar()) || (PhiR->getNumOperands() == 2 &&
+                                  isa<VPReductionRecipe>(PhiR->getOperand(1)));
     Type *VecTy =
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102