This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: match De Morgan's Law hidden by zext ops (PR22723)
ClosedPublic

Authored by spatel on Sep 8 2015, 1:58 PM.

Details

Summary

This is a fix for PR22723:
https://llvm.org/bugs/show_bug.cgi?id=22723

My first attempt at this was to change what I thought was the root problem:

xor (zext i1 X to i32), 1 --> zext (xor i1 X, true) to i32

...but we create the opposite pattern in InstCombiner::visitZExt(), so infinite loop!

My next idea was to fix the matchIfNot() implementation in PatternMatch, but that would mean potentially returning a different size for the match than what was input. I think this would require all users of m_Not to check the size of the returned match, so I abandoned that idea.

I settled on just fixing the exact case presented in the PR. I hope this limited patch is acceptable if not the most general. It does allow the 2 functions in PR22723 to compile identically (x86):

bool test(bool x, bool y) { return !x | !y; }
bool test(bool x, bool y) { return !x || !y; }
...
andb	%sil, %dil
xorb	$1, %dil
movb	%dil, %al
retq

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 34248.Sep 8 2015, 1:58 PM
spatel retitled this revision from to InstCombine: match De Morgan's Law hidden by zext ops (PR22723).
spatel updated this object.
spatel added reviewers: majnemer, bkramer, rtrieu.
spatel added a subscriber: llvm-commits.
sanjoy added inline comments.Sep 22 2015, 1:10 PM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1240 ↗(On Diff #34248)

Do you need to check if C1 fits in an uint64_t? Or is that checked earlier?

Actually, I think it'll be easier to just use C1->isOne().

spatel marked an inline comment as done.Sep 22 2015, 1:30 PM
spatel added inline comments.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1240 ↗(On Diff #34248)

Ah - I didn't know about that function. Fixed.

spatel updated this revision to Diff 35409.Sep 22 2015, 1:30 PM
spatel marked an inline comment as done.

Patch updated to use 'isOne()'.

hfinkel added inline comments.Sep 22 2015, 4:04 PM
test/Transforms/InstCombine/demorgan-zext.ll
14 ↗(On Diff #35409)

Please check here (and below) for the correct operands to these operations.

spatel updated this revision to Diff 35637.Sep 24 2015, 9:13 AM

Patch updated: check operands in tests.

hfinkel accepted this revision.Sep 25 2015, 2:40 PM
hfinkel edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Sep 25 2015, 2:40 PM
This revision was automatically updated to reflect the committed changes.