[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 a comment.

In D77635#1966625 <https://reviews.llvm.org/D77635#1966625>, @SjoerdMeijer wrote:

> I was also drafting a patch to implement this yesterday, and it was pretty much this! So I guess that's a good sign. :-)
>
> > @SjoerdMeijer , test tail-folding-counting-down.ll introduced in D72324 <https://reviews.llvm.org/D72324> now fails, as it can be vectorized with fold-tail, but is not vectorized due to cost. What's the intention of this test and how should it be changed?
>
> The purpose of this test was to catch a regression that we were seeing when tail-predication was rejected, but then incorrectly vectorisation also wasn't happening.
>  In this case, I think it is good to force vectorisation with a vectorisation vector of 4 or something along those lines. I was also modifying this test, but that will do for now, and then I will pick it up later.


Sorry for the confusion, was referring to the test under test/Transforms/LoopVectorize/ rather than the one under test/Transforms/LoopVectorize/ARM

OK, modified test to force vectorization with VF=4.

(Relevant for picking up later:) Note that llvm/test/Transforms/LoopVectorize/X86/small-size.ll also checks that a loop with reverse iv gets vectorized with fold-tail.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6772
 
     // Introduce the early-exit compare IV <= BTC to form header block mask.
     // This is used instead of IV < TC because TC may wrap, unlike BTC.
----------------
SjoerdMeijer wrote:
> nit: perhaps this comments now needs to be moved to line 6782, and we need to say something new/extra about the primary IV?
Added a line saying "Start by constructing the desired canonical IV."


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:333
+  /// Hold the primary induction phi of the vector loop.
+  Value *PrimaryInduction = nullptr;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1153
+  VPWidenPrimaryInductionRecipe() : VPRecipeBase(VPWidenPrimaryInductionSC) {
+    Val = new VPValue();
+  }
----------------
SjoerdMeijer wrote:
> nit: just curious, do we actually need this?
we need a VPValue; it could alternatively be owned by Plan, but seems better to hold it locally here.


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

https://reviews.llvm.org/D77635