Page MenuHomePhabricator

[InstCombine] reverse canonicalization of xor(zext i1 A), 1 <---> zext(not i1 A, true) (PR28476)
ClosedPublic

Authored by spatel on Jul 12 2016, 11:10 AM.

Details

Summary

We discussed this a bit in D21899.

By creating the official 'not' IR construct, we can:

  1. Remove DeMorgan-matching code that was added specifically to work-around the missing transform in http://reviews.llvm.org/rL248634.
  2. Make the DeMorgan transform for vectors too.
  3. Fix PR28476: https://llvm.org/bugs/show_bug.cgi?id=28476

I don't see any other transforms that were relying on the previous transform, so I think it's safe to replace it.

The test changes outside of the demorgan-zext.ll file look neutral to me; those just show that the xor gets pulled ahead of the zext. But we could make the argument that those transforms actually are improvements too because doing logic ops in a smaller type should always provide >= optimization opportunity to later passes.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 63694.Jul 12 2016, 11:10 AM
spatel retitled this revision from to [InstCombine] reverse canonicalization of xor(zext i1 A), 1 <---> zext(not i1 A, true) (PR28476).
spatel updated this object.
spatel added reviewers: eli.friedman, majnemer, hfinkel.
spatel added a subscriber: llvm-commits.
eli.friedman edited edge metadata.Jul 12 2016, 1:15 PM

Is "zext(a_bool ^ true) ^ 2" canonical, or is "zext(a_bool) ^ 3"? Either way, you're missing a transform.

Is "zext(a_bool ^ true) ^ 2" canonical, or is "zext(a_bool) ^ 3"? Either way, you're missing a transform.

Ah, I missed the point that you were making from your earlier comment. I'd say the ^3 version is canonical because less-instructions is better than 'not'.

I added tests for these patterns in rL275297 and rL275302 .

spatel updated this revision to Diff 64585.Jul 19 2016, 4:19 PM
spatel edited edge metadata.

Patch updated:
The deletions are identical to the earlier rev: (a) the specialized DeMorgan matcher (b) the canonicalization of zext before xor i1.

Instead of limiting this to only xor i1+zext, however, move any bitwise logic with a constant to the source type of the zext if the constant fits. I don't think there's any reason to limit the transform to i1 types. The vector case for doing this kind of transform came up in:
https://llvm.org/bugs/show_bug.cgi?id=28160

D22421 transforms the patterns that Eli noted.
D22477 handled the only existing test that I found that would have regressed from this change.

spatel updated this revision to Diff 64695.Jul 20 2016, 8:22 AM

Patch updated:
The previous rev forgot to delete some 'FIXME' comments from the test files.

eli.friedman accepted this revision.Jul 20 2016, 10:31 AM
eli.friedman edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 20 2016, 10:31 AM
This revision was automatically updated to reflect the committed changes.