[PATCH] D38948: [LV] Support efficient vectorization of an induction with redundant casts

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

A few additional minor comments..



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4378
+  // First insert the last instruction from the ExtTrunc cast sequence.
+  // The first instruction in the CastInsts set may be used outside of the
+  // ExtTrunc cast sequence, and therefore needs special handling later on.
----------------
Perhaps a clearer way of stating this: "This last instruction (is the only one which) may be used outside of the ExtTrunc cast sequence, so it is placed in position 0 of CastInsts, for special handing later on."

Worth asserting that CastInsts is empty before pushing back, to make sure that the above statement holds?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4404
+
+  CastInsts.push_back(AndInst);
+  return true;
----------------
assert empty before push_back?


================
Comment at: test/Transforms/LoopVectorize/vect-phiscev-sext-trunc.ll:26
+;
+; void doit3(int n, int step) {
+;   int i;
----------------
doit3 >> doit1


================
Comment at: test/Transforms/LoopVectorize/vect-phiscev-sext-trunc.ll:44
+; VF8-NOT: %[[TEST:[a-zA-Z0-9.]+]] = shl <8 x i32>
+; VF8-NOT: %{{.*}} = ashr exact <8 x i32> %[[TEST]]
+; VF8: <8 x i32>
----------------
If we first check NOT to have the shl which defines TEST, seems useless to then check that ashr doesn't use TEST.

Better check instead that the induction phi in vector.body (the one being stored into a[i]) feeds and consumes its bump directly?


================
Comment at: test/Transforms/LoopVectorize/vect-phiscev-sext-trunc.ll:94
+;
+; void doit3(int *in, int *out, size_t size, size_t step)
+; {
----------------
doit3 >> doit2


https://reviews.llvm.org/D38948