[PATCH] D21620: [LV] Don't widen trivial induction variables

[Adam Nemet via llvm-commits]

anemet added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2191
@@ +2190,3 @@
+
+  // We can't create a vector with less than two elements.
+  assert(VF > 1 && "VF should be greater than one");
----------------
mssimpso wrote:
> anemet wrote:
> > Matt, your argument about instcombine not being adequate for this is a convincing one.
> > 
> > On the other hand, I am still wondering if the solution is general enough.  There should be cases where we'd be better off having *both* the vector and the scalar version of the same induction variable.  I.e. depending on the use, you may want to use the vector version (e.g. store) or the scalar version (address calc, compares).
> > 
> > I am not saying that we need to necessarily implement this but the current solution should be taking us incrementally closer to the full solution.
> Adam,
> 
> One way to handle the cases you mention would be to allow the vectorizer to generate both a scalar version and a vector version of the same induction variable. Then it could select whether to generate a use of the scalar or vector version based on the instruction (address calculation, store, etc.). DCE and Instcombine would presumably clean up afterwards.
> 
> The current patch does take us incrementally to that kind of solution. It handles the case where we know we will always prefer to use the scalar version. If that's the case, we generate the scalar steps; otherwise, we continue to generate the vector steps. It doesn't, however, generate the scalar version of the induction variable along side the vector version.
> One way to handle the cases you mention would be to allow the vectorizer to generate both a scalar version and a vector version of the same induction variable. Then it could select whether to generate a use of the scalar or vector version based on the instruction (address calculation, store, etc.). DCE and Instcombine would presumably clean up afterwards.

Yes that is the direction I was thinking of going in the long term.  (If the later passes are ineffective cleaning up unused versions we can also generate them on demand.)

> The current patch does take us incrementally to that kind of solution. It handles the case where we know we will always prefer to use the scalar version. If that's the case, we generate the scalar steps; otherwise, we continue to generate the vector steps. It doesn't, however, generate the scalar version of the induction variable along side the vector version.

Fair enough.

I have one last high-level question.  Does this fix the pointer arithmetic code in https://llvm.org/bugs/show_bug.cgi?id=27881#c6 ?

I will look at the specifics of the patch later today.



http://reviews.llvm.org/D21620