Page MenuHomePhabricator

[InstSimplify] recognize trunc + icmp sgt/slt variants of select simplifications (PR28466)
ClosedPublic

Authored by spatel on Jul 19 2016, 2:54 PM.

Details

Summary

rL245171 exposed a hole in InstSimplify that manifested in a strange way in PR28466:
https://llvm.org/bugs/show_bug.cgi?id=28466

It's possible to use trunc + icmp sgt/slt in place of an and + icmp eq/ne, so we need to recognize that pattern to eliminate selects that are choosing between some value and some bitmasked version of that value.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 64567.Jul 19 2016, 2:54 PM
spatel retitled this revision from to [InstSimplify] recognize trunc + icmp sgt/slt variants of select simplifications (PR28466).
spatel updated this object.
spatel added reviewers: majnemer, eli.friedman.
spatel added a subscriber: llvm-commits.
eli.friedman edited edge metadata.Jul 19 2016, 4:26 PM

It looks like there are other places that look for bit-tests which are affected... see https://github.com/llvm-mirror/llvm/blob/dba9c3285e87768e80da6ca60f73b8913392dd98/lib/Transforms/InstCombine/InstCombineSelect.cpp#L579 , https://github.com/llvm-mirror/llvm/blob/65165b21da2aaed908f7bde1b8a16e7a8fe0c955/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp#L597 . Maybe we want a separate utility function to condense this functionality, so we don't end up with so many versions of it?

lib/Analysis/InstructionSimplify.cpp
3411 ↗(On Diff #64567)

Do we want to look through the truncate in something like "(trunc X) & C == C2 ? X : Y"?

3414 ↗(On Diff #64567)

getScalarSizeInBits? (This could be a vector select.)

It looks like there are other places that look for bit-tests which are affected... see https://github.com/llvm-mirror/llvm/blob/dba9c3285e87768e80da6ca60f73b8913392dd98/lib/Transforms/InstCombine/InstCombineSelect.cpp#L579 , https://github.com/llvm-mirror/llvm/blob/65165b21da2aaed908f7bde1b8a16e7a8fe0c955/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp#L597 . Maybe we want a separate utility function to condense this functionality, so we don't end up with so many versions of it?

Yikes - I didn't realize it was so blatantly copied in other places. That is pretty shameful. Let me see if there's a quick fix, otherwise I'll mark it as a FIXME for this patch.

lib/Analysis/InstructionSimplify.cpp
3411 ↗(On Diff #64567)

Is this the pattern in the last 4 tests of this patch, or something more general?

3414 ↗(On Diff #64567)

Yes - this was copied from the existing code. I was going to enable vectors as a follow-up to this patch.

eli.friedman added inline comments.Jul 20 2016, 10:04 AM
lib/Analysis/InstructionSimplify.cpp
3411 ↗(On Diff #64567)

I was thinking of a case where instead of trunc+slt, you have trunc+and+eq. Looking through a truncate isn't really specific to slt.

spatel added inline comments.Jul 20 2016, 1:55 PM
lib/Analysis/InstructionSimplify.cpp
3414 ↗(On Diff #64567)

But now it can't wait:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102583.html

AFAICT, the bug was present before the refactoring in rL275911, but nobody noticed until now.

spatel updated this revision to Diff 64948.Jul 21 2016, 1:51 PM
spatel edited edge metadata.

Patch updated:

  1. D22602 forced us to deal with the vector bug, so I've added some vector test cases.
  2. FIXME comments for refactoring were also added because D22602 exposed the code duplication sin.

I'd prefer to wait to add more patterns until I get some of the refactoring done. I don't see any quick fix here:
To refactor these cmp instruction analyzers properly, we should make the alternate code in decomposeBitTestICmp() vector-friendly and that means breaking artificial limits (dyn_cast<ConstantInt> vs. m_APInt()) on a very large number of transforms.

eli.friedman accepted this revision.Jul 21 2016, 1:58 PM
eli.friedman edited edge metadata.

LGTM for now, I guess.

This revision is now accepted and ready to land.Jul 21 2016, 1:58 PM
This revision was automatically updated to reflect the committed changes.