[PATCH] D77635: [LV] Vectorize with FoldTail when Primary Induction is absent

[Ayal Zaks via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:333
+  /// Hold the primary induction phi of the vector loop.
+  Value *PrimaryInduction = nullptr;
+
----------------
SjoerdMeijer wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Maybe include Vector in the name, e.g. VectorInduction, to avoid confusion with Legal's PrimaryInduction
> > Agreed. Updated comment and changed name to VectorLoopScalarIV. Sounds better/ok?
> > 
> > Maybe better to avoid overloading the "PrimaryInduction" name throughout, keeping it with its original meaning only, and use "Canonical" instead.
> We are well into bikeshedding territory now, but just one quick question.  VectorLoopScalarIV describes well what this is I think,  but was just wondering that if we refer to this as the canonical IV in comments, should this not be named something with CanonicalIV in it?
Yes, I was wondering above if we should use (Scalar/Vector) `CanonicalIV` throughout, instead of overloading the original `PrimaryInduction`, which is **absent**... uploading another version to see how it looks.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1148
+class VPWidenPrimaryInductionRecipe : public VPRecipeBase {
+private:
+  VPValue *Val = nullptr;
----------------
fhahn wrote:
> nit: private not needed here, right? Classes should default to private
right, trying to be consistent...
They should all be dropped in a separate NFC patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1149
+private:
+  VPValue *Val = nullptr;
+
----------------
fhahn wrote:
> fhahn wrote:
> > Could be a unique_ptr?
> actually, the lifetime is directly tied to the recipe, right? So maybe no pointer is needed at all and we can just add a `VPValue Val` member?
Right! Lifetimes are indeed tied; when the recipe turns into a VPInstruction, the VPValue will coincide with 'this'. Good catch.


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

https://reviews.llvm.org/D77635