This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] make icmp vector canonicalization safe for constant with undef elements
ClosedPublic

Authored by spatel on Oct 28 2019, 10:26 AM.

Details

Summary

This is a fix for:
https://bugs.llvm.org/show_bug.cgi?id=43730
...and as shown there, we have an existing test case that shows a potential miscompile.

We could just bail out for vector constants that contain any undef elements, or we can do as shown here:
allow the transform, but replace the undefs with a safe value.

For the tests shown, this results in a full splat constant (no undefs) which is probably a win for further IR analysis because we conservatively don't match undefs in most cases. Codegen can probably recover these kinds of undef lanes via demanded elements analysis if that's profitable.

Diff Detail

Event Timeline

spatel created this revision.Oct 28 2019, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 10:26 AM
lebedev.ri added inline comments.Oct 28 2019, 10:32 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5197–5208

Time to hoist replaceUndefsWith() from llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp ?

spatel marked an inline comment as done.Oct 28 2019, 10:44 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5197–5208

Yes - I forgot where that was hiding. :)

spatel updated this revision to Diff 226728.Oct 28 2019, 12:31 PM

Patch updated:
Hoist the existing function for replacing undefs and use it. I can make that part an NFC pre-commit, but I kept it all together here in the review for the context.

lebedev.ri accepted this revision.Oct 28 2019, 12:47 PM

I didn't verify that alive no longer barfs on that transform,
but i do believe the change makes sense in the context of
the example provided in that bugreport.

Should there be a icmp sge test?

Other than that, thanks, LG.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5190

This will result in the last one being used

5198

with last safe constant we found

This revision is now accepted and ready to land.Oct 28 2019, 12:47 PM
spatel marked an inline comment as done.Oct 29 2019, 4:37 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5190

Good catch. Yes, the test coverage isn't sufficient. I'll add tests before splitting/committing/fixing.

This revision was automatically updated to reflect the committed changes.