[PATCH] D104197: [VPlan] Track both incoming values for first-order recurrence phis.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4760
 
-  VPValue *StartVPV = PhiR->getStartValue();
+  VPValue *StartVPV = RdxDesc ? PhiR->getStartValue() : nullptr;
   Value *StartV = StartVPV ? StartVPV->getLiveInIRValue() : nullptr;
----------------
Ayal wrote:
> This does raise an eyebrow... rename StartVPV and StartV to something like ReductionStartVPV and ReductionStartV? Only start values of reductions are handled here, those of firstOrderRecurrences are handled in fixFirstOrderRecurrence().
> 
> Better yet to first handle firstOrderRecurrences, separately? It's just creating the phi (which will eventually be replaced by the one fixFirstOrderRecurrence() creates) and setting the state, per part.
I re-organized the code a bit more. WDYT?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8976
+      PhiRecipe = new VPWidenPHIRecipe(Phi);
+      PhiRecipe->addOperand(StartV);
+      if (Legal->isFirstOrderRecurrence(Phi)) {
----------------
Ayal wrote:
> This StartV for non-reduction phi's is currently not being used, except perhaps as a placeholder to ensure backedge takes 2nd place, right? Should potentially serve fixFirstOrderRecurrence()?
I updated the patch to use both the start and backedge VPValue in fixFirstOrderRecurrence.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8977
+      PhiRecipe->addOperand(StartV);
+      if (Legal->isFirstOrderRecurrence(Phi)) {
+        Value *Incoming =
----------------
Ayal wrote:
> What's left as 'else' here, pointer inductions? Worth a TODO to model their backedges as well?
Added the todo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104197