This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] canonicalize icmp predicate feeding select
ClosedPublic

Authored by spatel on Jun 15 2017, 9:32 AM.

Details

Summary

This canonicalization was suggested in D33172 as a way to make InstCombine behavior more uniform. We have this transform for icmp+br, so unless there's some reason that icmp+select should be treated differently, I think we should do the same thing here.

The benefit comes from increasing the chances of creating identical instructions. This is shown in the tests in logical-select.ll (PR32791). InstCombine doesn't fold those directly, but EarlyCSE can simplify the identical cmps, and then InstCombine can fold the selects together.

The possible regression for the tests in select.ll raises questions about poison/undef:
http://lists.llvm.org/pipermail/llvm-dev/2017-May/113261.html

...but I think that transform is just as likely to be triggered by this canonicalization as it is to be missed, so we're just pointing out a commutation deficiency in the pattern matching:
https://reviews.llvm.org/rL228409

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jun 15 2017, 9:32 AM
davide edited edge metadata.Jun 17 2017, 4:33 PM

If the goal of this is to expose more opportunities for congruent expressions maybe GVN itself should do this canonicalization, maybe? Some algorithms perform, e.g., reassociation to catch more cases.
See, e.g.
http://www.sable.mcgill.ca/~hendren/621/karthikhandouts.pdf

If the goal of this is to expose more opportunities for congruent expressions maybe GVN itself should do this canonicalization, maybe? Some algorithms perform, e.g., reassociation to catch more cases.
See, e.g.
http://www.sable.mcgill.ca/~hendren/621/karthikhandouts.pdf

Are you suggesting a change to GVN as an independent improvement or as a substitute for this transform in InstCombine? As I mentioned, I think this patch makes InstCombine behavior more predictable and consistent (because we already have this transform for cmp+br).

efriedma accepted this revision.Jun 26 2017, 5:00 PM

LGTM.

This revision is now accepted and ready to land.Jun 26 2017, 5:00 PM
This revision was automatically updated to reflect the committed changes.