Page MenuHomePhabricator

[InstCombine] Recognize and simplify three way comparison idioms
ClosedPublic

Authored by anna on Jun 16 2017, 6:53 AM.

Details

Summary

Many languages have a three way comparison idiom where comparing two values
produces not a boolean, but a tri-state value. Typical values (e.g. as used in
the lcmp/fcmp bytecodes from Java) are -1 for less than, 0 for equality, and +1
for greater than.

We actually do a great job already of converting three way comparisons into
binary comparisons when the result produced has one a single use. Unfortunately,
such values can have more than one use, and in that case, our existing
optimizations break down.

The patch adds a peephole which converts a three-way compare + test idiom into a
binary comparison on the original inputs. It focused on replacing the test on
the result of the three way compare and does nothing about removing the three
way compare itself. That's left to other optimizations (which do actually kick
in commonly.)
We currently recognize one idiom on signed integer compare. In the future, we
plan to recognize and simplify other comparison idioms on
other signed/unsigned datatypes such as floats, vectors etc.

This is a resurrection of Philip Reames' original patch:
https://reviews.llvm.org/D19452

Diff Detail

Repository
rL LLVM

Event Timeline

anna created this revision.Jun 16 2017, 6:53 AM
anna added a comment.Jun 16 2017, 6:57 AM

Changed wrt Philip's original patch:

  1. Adapted to get working with current API (obviously ;) )
  2. Addressed review comments by Sanjoy.
  3. CHECK pattern on tests were modified -- not a semantic modification, just syntactic, because we generate tmp compares now, not hardcoded compares.
mkazantsev added inline comments.Jun 21 2017, 11:03 PM
lib/Transforms/InstCombine/InstCombineCompares.cpp
2454 ↗(On Diff #102820)

We can check PredA right after we matched line 2450 to avoid further maching if we have a wrong comparison there.

2454 ↗(On Diff #102820)

Maybe just another && instead of second if?

2474 ↗(On Diff #102820)

Can we assert C on method entry?

2495 ↗(On Diff #102820)

Is it possible that more than one of {TrueWhenLessThan, TrueWhenEqual, TrueWhenGreaterThan} is true?

If no, please add an assertion on that and use "else if" below.
If yes, should we not consider this when creating conditions?

anna added inline comments.Jun 22 2017, 6:12 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
2454 ↗(On Diff #102820)

agreed.

2474 ↗(On Diff #102820)

yes. I'll move it.

2495 ↗(On Diff #102820)

Yes, more than one can be true at a time: this actually enumerates all the possible 8 combinations, because of the chaining of these ORs in CreateOr. We pass in the Cond as the parameter.

(I actually liked this idea a lot: credit goes to Sanjoy in suggesting this in the original review).

Please see the comment above the code, should I perhaps clarify it more?

mkazantsev added inline comments.Jun 22 2017, 9:55 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
2495 ↗(On Diff #102820)

I've re-read the comment and the code, and I think it's fine, I just didn't realize the idea from the very beginning.

anna updated this revision to Diff 103612.Jun 22 2017, 11:44 AM
anna marked 2 inline comments as done.

addressed comments.

mkazantsev accepted this revision.Jun 22 2017, 9:30 PM

LGTM.

lib/Transforms/InstCombine/InstCombineCompares.cpp
2441 ↗(On Diff #103612)

Why do we need this blank line?

2507 ↗(On Diff #103612)

Same here.

This revision is now accepted and ready to land.Jun 22 2017, 9:30 PM
This revision was automatically updated to reflect the committed changes.
anna marked 2 inline comments as done.Jun 23 2017, 6:43 AM