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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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;
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8976
+      PhiRecipe = new VPWidenPHIRecipe(Phi);
+      PhiRecipe->addOperand(StartV);
+      if (Legal->isFirstOrderRecurrence(Phi)) {
----------------
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()?


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8980
+            Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch());
+        Instruction *IncI = dyn_cast<Instruction>(Incoming);
+        recordRecipeOf(IncI);
----------------
dyn_cast may return null; use cast<>?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8982
+        recordRecipeOf(IncI);
+        PhisToFix.push_back(PhiRecipe);
+      }
----------------
Comments in VPRecipeBuilder.h that PhisToFix handles **reduction** phi's only should be updated, to include FOR phi's as well:

```
  /// Cross-iteration **reduction** phis for which we need to add the incoming value
  /// from the backedge after all recipes have been created.
  SmallVector<VPWidenPHIRecipe *, 4> PhisToFix;
```


```
  /// Add the incoming values from the backedge to **reduction** cross-iteration
  /// phis.
  void fixHeaderPhis();
```

Comments in VPlan.h about the backedge of VPWidenPHIRecipe should be updated as well:

```
/// For **reduction** PHIs, RdxDesc must point to the corresponding recurrence
/// descriptor, the start value is the first operand of the recipe and the
/// incoming value from the backedge is the second operand.
```


```
  /// Returns the incoming value from the loop backedge, if it is a **reduction**.
  VPValue *getBackedgeValue()
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104197