[PATCH] D41913: [LV] Don't call recordVectorLoopValueForInductionCast for newly-created IV from a trunc.

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr35773.ll:34-36
+; TODO: Not sure why these are generated, but this is a test for a crash, so leave as a TODO:
+; CHECK-NEXT:    [[BROADCAST_SPLATINSERT:%.*]] = insertelement <4 x i32> undef, i32 [[MAIN_IV]], i32 0
+; CHECK-NEXT:    [[BROADCAST_SPLAT:%.*]] = shufflevector <4 x i32> [[BROADCAST_SPLATINSERT]], <4 x i32> undef, <4 x i32> zeroinitializer
----------------
a.elovikov wrote:
> mssimpso wrote:
> > Currently, every induction variable gets vectorized in some way, even though we may know that the vector version will not be needed. So even if we know the induction variable is uniform, as %main.iv is in this case, we will still vectorize it with broadcast/splats, knowing that InstCombine will simplify it all. The vectorizer relies on InstCombine for a lot of cleanup, actually. But I think we're to the point where we don't need to do this, since we can build up vector values on-demand if needed (this wasn't always the case). You can add "-instcombine" to your run line here if you'd rather just check the cleaner output. This is done in many test cases.
> > 
> > For the TODO, it's probably better to add something like that in the code for greater visibility, instead of in the test case. The relevant piece of code is the block at the end of `widenIntOrFpInduction` that begins with `if (!VectorizedIV)`. This could be added as a separate patch I think.
> instcombine changes the output way too much, IMO. I'd rather keep the RUN line and checks as is because they describe what exactly the vectorizer is doing.  I'll remove the "TODO" in such case.
> 
> Another possibility is to add "-instcombine" but don't run FileCheck at all - just verify that "opt" is not crashing. Not sure if that's common in the tests though.
> instcombine changes the output way too much, IMO. I'd rather keep the RUN line and checks as is because they describe what exactly the vectorizer is doing. I'll remove the "TODO" in such case.

Sounds good to me, but feel free to add a relevant TODO to the .cpp if you'd like. I'm pretty sure we can avoid generating these unneeded sequences, but probably no one has had the time or interest to look at that yet.

> Another possibility is to add "-instcombine" but don't run FileCheck at all - just verify that "opt" is not crashing. Not sure if that's common in the tests though.

I'd prefer that we not do this. We should definitely check that were getting the expected output.


https://reviews.llvm.org/D41913