Page MenuHomePhabricator

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

Authored by a.elovikov on Jan 10 2018, 11:35 AM.

Details

Summary

This method is supposed to be called for IVs that have casts in their use-def
chains that are completely ignored after vectorization under PSE. However, for
truncates of such IVs the same InductionDescriptor is used during
creation/widening of both original IV based on PHINode and new IV based on
TruncInst.

This leads to unintended second call to recordVectorLoopValueForInductionCast
with a VectorLoopVal set to the newly created IV for a trunc and causes an
assert due to attempt to store new information for already existing entry in the
map. This is wrong and should not be done.

Fixes PR35773.

Diff Detail

Repository
rL LLVM

Event Timeline

a.elovikov created this revision.Jan 10 2018, 11:35 AM
dim added a subscriber: dim.Jan 11 2018, 7:23 AM

For me, this fixes both minimized test cases mentioned in PR35773, and the full test cases originating from https://bugs.freebsd.org/224867 and https://bugs.freebsd.org/224868.

Hi Dimitry,

This looks good to me, but it's probably a good idea to let Dorit have a look as well. Also, this should be a release blocker if it's not already. Thanks!

llvm/test/Transforms/LoopVectorize/pr35773.ll
34–36 ↗(On Diff #129315)

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.

dim added a comment.Jan 11 2018, 1:10 PM

This looks good to me, but it's probably a good idea to let Dorit have a look as well. Also, this should be a release blocker if it's not already. Thanks!

Yes, PR35773 is marked as release blocker, and blocks PR35804 (the 6.0.0 release meta-bug).

Also, this should be a release blocker if it's not already. Thanks!

Does it mean that I'll be required to commit the fix into some other branch after it lands to trunk?

llvm/test/Transforms/LoopVectorize/pr35773.ll
34–36 ↗(On Diff #129315)

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.

Also, this should be a release blocker if it's not already. Thanks!

Does it mean that I'll be required to commit the fix into some other branch after it lands to trunk?

No extra commit should be necessary. Once it's committed to trunk, make a note of the revision on the bugzilla in the "Fixed By Commit" field. PR35773 is already blocking 6.00 so if you don't resolve the ticket then the release team should see it and they can resolve it once they've merged.

mssimpso added inline comments.Jan 11 2018, 1:56 PM
llvm/test/Transforms/LoopVectorize/pr35773.ll
34–36 ↗(On Diff #129315)

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.

  • Move TODO from the test to the actual code.
dorit accepted this revision.Jan 14 2018, 2:46 AM

LGTM. Thanks for the fix.

This revision is now accepted and ready to land.Jan 14 2018, 2:46 AM
This revision was automatically updated to reflect the committed changes.