This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] add isCanonicalPredicate() helper function and use it; NFCI
ClosedPublic

Authored by spatel on May 16 2017, 10:37 AM.

Details

Summary

There should be a slight efficiency improvement from handling icmp/fcmp with one matcher and reducing duplicated code.

The larger motivation is that there are questions about how predicate canonicalization is handled, and the refactoring should make it easier if we want to change any of that behavior.

I'll post those questions here for reference, but I expect we should answer these in follow-up patches and/or llvm-dev:

  1. As noted in the code comment, we've chosen 3 of the 16 FCMP preds as not canonical. Why those 3? It goes back to rL32751 from what I can tell, but I'm not sure if there's a justification for that rule.
  2. We currently do not canonicalize most select conditions. Should we use the same rule that applies to branches for selects?
  3. We currently do canonicalize some FP select conditions, and those rules would conflict with the rule shown here. Should one or both be changed? Canonicalize to use ordered comparisons by swapping the select operands. e.g. // (X ugt Y) ? X : Y -> (X ole Y) ? Y : X

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.May 16 2017, 10:37 AM
davide added inline comments.May 16 2017, 10:43 AM
lib/Transforms/InstCombine/InstCombineInternal.h
88–92 ↗(On Diff #99164)

Given that was about 11 years ago, it's unlikely we're going to find out.
That said, I'm not sure this is relevant anymore so we might reconsider.

95–96 ↗(On Diff #99164)

I think GCC might complain about this so you might consider adding an llvm_unreachable at the end of this.

spatel added inline comments.May 16 2017, 10:58 AM
lib/Transforms/InstCombine/InstCombineInternal.h
95–96 ↗(On Diff #99164)

Do you know of a case where that happens? I couldn't find one:
https://godbolt.org/g/l0XnXd
...so I'd rather not ugly up the code for something if it doesn't really happen. We could 'default: break' and return true outside of the switch if you think that reads better.

davide added inline comments.May 16 2017, 11:03 AM
lib/Transforms/InstCombine/InstCombineInternal.h
95–96 ↗(On Diff #99164)

I saw it emitting warning in similar cases, but not this one (just checked).
If neither compiler complains, I think you can leave as is (worst case we'll need to fix a builbot failure, not a big deal).

craig.topper added inline comments.
lib/Transforms/InstCombine/InstCombineInternal.h
95–96 ↗(On Diff #99164)

I think gcc's complaint is normally when the switch covers all values of an enum and there is no default case.

From the coding standards:

"A knock-on effect of this stylistic requirement is that when building LLVM with GCC you may get warnings related to “control may reach end of non-void function” if you return from each case of a covered switch-over-enum because GCC assumes that the enum expression may take any representable value, not just those of individual enumerators. To suppress this warning, use llvm_unreachable after the switch."

spatel updated this revision to Diff 99192.May 16 2017, 2:09 PM

Patch updated:
Although this patch is NFCI, we had exactly zero tests for the FCMP transforms, and the ICMP transforms were not tested completely. Given the lumpy handling of the predicates, I think it's best to check all of them to make sure we're doing exactly what we think we're doing.

I checked that there is no difference for the tests before/after this code change, but I can check in the tests first if that would be preferred.

davide accepted this revision.May 16 2017, 2:13 PM

Patch updated:
Although this patch is NFCI, we had exactly zero tests for the FCMP transforms, and the ICMP transforms were not tested completely. Given the lumpy handling of the predicates, I think it's best to check all of them to make sure we're doing exactly what we think we're doing.

I checked that there is no difference for the tests before/after this code change, but I can check in the tests first if that would be preferred.

I think adding these tests is a good thing. I don't think it's worth checking in them separately.
I guess this is fine, but please wait for either Eli or David to give another look.

This revision is now accepted and ready to land.May 16 2017, 2:13 PM
efriedma accepted this revision.May 16 2017, 4:04 PM

Cleanup looks fine.

lib/Transforms/InstCombine/InstCombineInternal.h
91 ↗(On Diff #99192)

Not sure why these three are special either... from a brief look at the commit, I would guess it was done to mirror the integer transforms, without any real consideration for what should be canonical.

If I were doing it from scratch, I might say we should consider ordered predicates canonical, since they're the ones people are used to working with.

This revision was automatically updated to reflect the committed changes.