[PATCH] D29956: [LV] Remove constant-step restriction for vector phi creation

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Thanks, Matt!
A few minor comments inline.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2376
     auto *TruncType = cast<IntegerType>(EntryVal->getType());
-    Step = ConstantInt::getSigned(TruncType, Step->getSExtValue());
+    Step = Builder.CreateSExtOrTrunc(Step, TruncType);
     Start = Builder.CreateCast(Instruction::Trunc, Start, TruncType);
----------------
Can this really be anything but a trunc? If not, why not CreateTrunc?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2384
+  // non-constant, we create the vector splat with IRBuilder. IRBuilder can
+  // constant-fold the multiply, but it doesn't handle a constant vector splat.
+  auto *ConstVF = ConstantInt::getSigned(Start->getType(), VF);
----------------
Maybe it should? :-)
(I wouldn't block this patch on that, but please mark this as a FIXME.)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2466
+    SCEVExpander Exp(*PSE.getSE(), DL, "induction");
+    Step = Exp.expandCodeFor(ID.getStep(), ID.getStep()->getType(),
+                             LoopVectorPreHeader->getTerminator());
----------------
This doesn't add any new overhead - we'd expand this SCEV into the pre-header anyway, for the scalar induction, right?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2489
       ScalarIV = Builder.CreateCast(Instruction::Trunc, Induction, TruncType);
-      Step = ConstantInt::getSigned(TruncType, StepInt);
+      Step = Builder.CreateSExtOrTrunc(Step, TruncType);
     } else {
----------------
Same as above.


================
Comment at: test/Transforms/LoopVectorize/induction-step.ll:201
+  ret void
+}
----------------
Do we have a negative test-case showing we don't try anything stupid for loop-variant values?


https://reviews.llvm.org/D29956