This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] analyze (optionally casted) icmps to eliminate obviously false logic (PR27869)
ClosedPublic

Authored by spatel on Jun 19 2016, 3:06 PM.

Details

Summary

By moving this transform to InstSimplify from InstCombine, we sidestep the problem/question raised by PR27869:
https://llvm.org/bugs/show_bug.cgi?id=27869
...where InstCombine turns an icmp+zext into a shift causing us to miss the fold.

Diff Detail

Event Timeline

spatel updated this revision to Diff 61230.Jun 19 2016, 3:06 PM
spatel retitled this revision from to [InstSimplify] look through zexts of icmps to eliminate obviously false logic (PR27869).
spatel updated this object.
spatel added reviewers: eli.friedman, majnemer, sanjoy.
spatel added a subscriber: llvm-commits.
majnemer edited edge metadata.Jun 19 2016, 3:22 PM

Hmm. More generally, how about having and of zexts get treated as zext of ands?

Hmm. More generally, how about having and of zexts get treated as zext of ands?

I think we already do that, but the case in the bug report shows an example where one of the zexts is gone before we reach the and.

Hmm. More generally, how about having and of zexts get treated as zext of ands?

I think we already do that, but the case in the bug report shows an example where one of the zexts is gone before we reach the and.

I don't believe so, what I imagined was something like the following: https://ghostbin.com/paste/fdc72

Hmm. More generally, how about having and of zexts get treated as zext of ands?

I think we already do that, but the case in the bug report shows an example where one of the zexts is gone before we reach the and.

I don't believe so, what I imagined was something like the following: https://ghostbin.com/paste/fdc72

Ah - you meant here in InstSimplify; I was still thinking InstCombine. This looks nice. I was wondering how to make the cast without creating an instruction, and your paste answers that: use ConstantExpr::getCast().

Let me add more tests and update. Thanks!

spatel updated this revision to Diff 61284.Jun 20 2016, 12:18 PM
spatel retitled this revision from [InstSimplify] look through zexts of icmps to eliminate obviously false logic (PR27869) to [InstSimplify] analyze (optionally casted) icmps to eliminate obviously false logic (PR27869).
spatel updated this object.
spatel edited edge metadata.

Patch updated:

  1. Use slightly modified version of David's suggested code to make the transform more general.
  2. Add tests to cover more possible cases and some negative cases.
  3. Remove the equivalent transform from InstCombine and move its regression tests to InstSimplify.

This now includes what I thought would be the follow-up step in one patch. I can make step #3 a separate commit if that seems safer. Or if I'm wrong in assuming that everything in the InstCombine path is now handled by InstSimplify, please let me know.

majnemer accepted this revision.Jun 20 2016, 12:32 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 20 2016, 12:32 PM
This revision was automatically updated to reflect the committed changes.