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

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
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);
----------------
mkuper wrote:
> Can this really be anything but a trunc? If not, why not CreateTrunc?
It's always a trunc - I thought about that after submitting - I'll update it.


================
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);
----------------
mkuper wrote:
> Maybe it should? :-)
> (I wouldn't block this patch on that, but please mark this as a FIXME.)
I agree. I'll add FIXME here for now.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2466
+    SCEVExpander Exp(*PSE.getSE(), DL, "induction");
+    Step = Exp.expandCodeFor(ID.getStep(), ID.getStep()->getType(),
+                             LoopVectorPreHeader->getTerminator());
----------------
mkuper wrote:
> This doesn't add any new overhead - we'd expand this SCEV into the pre-header anyway, for the scalar induction, right?
Right, no new overhead here. We're just expanding early if the step is loop-invariant. The existing expansion is performed a bit further down at line 2497 (for the scalar IVs and the splat vector IVs).

The insertion point for the existing expansion has always been the current IRBuilder insertion point, which is actually within the body of the loop. But for a loop-invariant step, SCEVExpander is smart enough to place it in the pre-header.


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


https://reviews.llvm.org/D29956