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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:586
+  void fixFirstOrderRecurrence(VPFirstOrderRecurrencePHIRecipe *PhiR,
+                               VPTransformState &State);
 
----------------
Ayal wrote:
> This could be applied independently.
I'll push it as a separate commit


================
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
----------------
Ayal wrote:
> A [pure virtual] base class
> 
> inductions[.]and
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1069
   /// List of incoming blocks. Only used in the VPlan native path.
   SmallVector<VPBasicBlock *, 2> IncomingBlocks;
 
----------------
Ayal wrote:
> Keep IncomingBlocks in VPWidenPHIRecipe?
Sounds good, thanks! Let's keep the base class as lightweight as possible.


================
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() {
----------------
Ayal wrote:
> "if there is one" - getOperand(1) asserts that there is one...
Removed.


================
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);
----------------
Ayal wrote:
> Indep, of this patch, but why not have one constructor:
> 
> ```
>   VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr)
>       : VPHeaderPHIRecipe(VPVWidenPHISC, VPWidenPHISC, Phi, Start) {}
> ```
Good point! I rather remove the (single?) users of the constructor without start value as follow-up


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116304