[PATCH] D20315: [LV] For some induction variables, use vector phis instead of widening the scalar in the loop body

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2121
@@ +2120,3 @@
+  auto CurrIP = Builder.saveIP();
+  Builder.SetInsertPoint(LoopVectorPreHeader->getTerminator());
+  if (TruncType) {
----------------
delena wrote:
> I assume that loop invariant code will be hoisted anyway.
Probably, but since we know this belongs in the preheader, I'd prefer to put it there to begin with, if it's easy - and it seems like it is.
Or are you afraid this may be wrong?

(The reason I'm doing the saveIP/restoreIP dance instead of just creating a new IRBuilder is that getStepVector expects Builder to point at the right location. I can refactor this to have getStepVector accept an IRBuilder parameter instead, if you think that'll look better.)

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4295
@@ +4294,3 @@
+          IntegerType *TruncType = cast<IntegerType>(CI->getType());
+          StepValue = ConstantInt::getSigned(TruncType, StepValue->getSExtValue());
+          if (VF == 1) {
----------------
delena wrote:
> this line should be moved down, under the if (VF==1)
Right, thanks.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4306
@@ +4305,3 @@
+            // a new, truncated, vector IV based on that.
+            assert(II.getConstIntStepValue() &&
+              "Primary induction variable should have a const step");
----------------
delena wrote:
> you don't need assert here, you are under the "if"
Right, this was just for equivalence with the other callsite. I'll sink the assert into the function, like you suggested above.


http://reviews.llvm.org/D20315