[PATCH] D116304: [VPlan] Add abstract base class for header phi recipes (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:586
+  void fixFirstOrderRecurrence(VPFirstOrderRecurrencePHIRecipe *PhiR,
+                               VPTransformState &State);
 
----------------
This could be applied independently.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1063
+/// A base class for all recipes modeling header phis, including phis for first
+/// order recurrences, pointer inductions.and reductions. The start value is the
+/// first operand of the recipe and the incoming value from the backedge is the
----------------
A [pure virtual] base class

inductions[.]and


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1069
   /// List of incoming blocks. Only used in the VPlan native path.
   SmallVector<VPBasicBlock *, 2> IncomingBlocks;
 
----------------
Keep IncomingBlocks in VPWidenPHIRecipe?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1094
 
   /// Generate the phi/select nodes.
+  void execute(VPTransformState &State) override = 0;
----------------
phi/select >> phi


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1103
 
-  /// Returns the start value of the phi, if it is a reduction or first-order
-  /// recurrence.
+  /// Returns the start value of the phi, it one is set.
   VPValue *getStartValue() {
----------------
it one >> if one


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1108
 
-  /// Returns the incoming value from the loop backedge, if it is a reduction or
-  /// first-order recurrence.
+  /// Returns the incoming value from the loop backedge, if there is one.
   VPValue *getBackedgeValue() {
----------------
"if there is one" - getOperand(1) asserts that there is one...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1139
+  /// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start.
+  VPWidenPHIRecipe(PHINode *Phi, VPValue &Start) : VPWidenPHIRecipe(Phi) {
+    addOperand(&Start);
----------------
Indep, of this patch, but why not have one constructor:

```
  VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr)
      : VPHeaderPHIRecipe(VPVWidenPHISC, VPWidenPHISC, Phi, Start) {}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116304