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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks a lot for cleaning up my mess, the original patch was probably rather overzealous. :-\
The cases I've looked at were nicely cleaned up by InstCombine, but I guess having it work in the general case was too much to expect.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2189
@@ +2188,3 @@
+
+  // We can't create a vector with less than two elements.
+  assert(VF > 1 && "VF should be greater than one");
----------------
This looks a bit weird to me.Do you have a test-case where this works, but instcombine fails to simplify the regular step vector?
Perhaps it would be better to fix instcombine, I can look into it.

In any case, that shouldn't block the rest of the patch - (a) not widening the phi when the uses are trivial, and (b) using the scalarized step vector instead of the regular one should be independently good, right?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4145
@@ +4144,3 @@
+
+  // Look at the users of the induction variable inside the loop, and determine
+  // if any of them are interesting. If we find interesting users (e.g., loads
----------------
We are already doing something very similar in collectValuesToIgnore(), for basically the same reason - to estimate whether we'll end up needing a vector IV or not (and when InstCombine can't do the cleanup, my patch breaks that logic as well).

Can you use collectValuesToIgnore() instead? Either by checking whether all the phi users are in ValuesToIgnore, or by marking the PHI itself as "should/should not be widened" during collectValuesToIgnore(), if that makes sense?



http://reviews.llvm.org/D21620