Page MenuHomePhabricator

[InstCombine] Improve the expansion in SimplifyUsingDistributiveLaws to handle cases where one side doesn't simplify, but the other side resolves to an identity value

Authored by craig.topper on Jul 15 2017, 12:05 AM.



If one side simplifies to the identity value for inner opcode, we can replace the value with just the operation that can't be simplified.

I've removed a couple now unneeded special cases in visitAnd and visitOr. There are probably other cases I missed.

@spatel I believe this fixes your hidden not test cases with compares. If you send over your test cases I can add them to this patch.

Diff Detail


Event Timeline

craig.topper created this revision.Jul 15 2017, 12:05 AM
spatel edited edge metadata.Jul 15 2017, 6:48 AM

Thanks. I hadn't gotten to the first pattern that you're fixing/deleting, but these tests should cover the 2nd and 3rd.

grandinj added inline comments.
653 ↗(On Diff #106755)

the do -> they do

662 ↗(On Diff #106755)

the do -> they do

690 ↗(On Diff #106755)


699 ↗(On Diff #106755)


Added Sanjay's compare test cases.

spatel accepted this revision.Jul 15 2017, 1:12 PM

There's an open question about whether InstCombine should be in the refactoring/distribution optimization business in the first place. However, given that this patch both improves that folding and reduces specialized matchers, it will make it easier to lift this code into a different pass when we decide to move this.


651 ↗(On Diff #106776)

"Do" -> "Does" (and same for the comments below)

653 ↗(On Diff #106776)

"They do!" -> "It does!" (and repeat below)

698 ↗(On Diff #106776)

Should be "Commute the 'and':"
Also reverse the other similar comments; these got inverted from the 'or' tests.

753 ↗(On Diff #106776)

This is a copy of my mistake in the original comment, so it's repeated 8 times I think: the result is actually (~X & Y).

This revision is now accepted and ready to land.Jul 15 2017, 1:12 PM
This revision was automatically updated to reflect the committed changes.