Page MenuHomePhabricator

Added inst combine transforms for single bit tests from Chris's note (updated)
ClosedPublic

Authored by dinesh.d on May 15 2014, 2:01 AM.

Details

Summary

I had to revert r208848 [http://reviews.llvm.org/D3717] due to assertion during builds :(

"include/llvm/ADT/APInt.h:960: bool llvm::APInt::operator==(const llvm::APInt&) const: Assertion `BitWidth == RHS.BitWidth && "Comparison requires equal bit widths"' failed."

I have updated patch to fix this issue. Now I am using APInt::isSameValue for comparison, which should be ok as these numbers are used in bitwise operations.

Is it ok now?

Summary from http://reviews.llvm.org/D3717
This patch adds transforms for

Single bit tests [http://nondot.org/sabre/LLVMNotes/InstCombine.txt]:
if ((x & C) == 0) x |= C becomes x |= C
if ((x & C) != 0) x ^= C becomes x &= ~C
if ((x & C) == 0) x ^= C becomes x |= C
if ((x & C) != 0) x &= ~C becomes x &= ~C
if ((x & C) == 0) x &= ~C becomes nothing

Z3 Verifications code for above transform
http://rise4fun.com/Z3/Pmsh

Diff Detail

Repository
rL LLVM

Event Timeline

dinesh.d updated this revision to Diff 9416.May 15 2014, 2:01 AM
dinesh.d retitled this revision from to Added inst combine transforms for single bit tests from Chris's note (updated).
dinesh.d updated this object.
dinesh.d edited the test plan for this revision. (Show Details)
dinesh.d added reviewers: bkramer, rafael.
dinesh.d added a subscriber: Unknown Object (MLST).
dinesh.d updated this revision to Diff 9419.May 15 2014, 2:20 AM

Number should be ZExted before taking NOT for comparison.

bkramer edited edge metadata.May 18 2014, 5:33 AM

I don't understand why the bit width can differ here. There might be something else wrong with your patch. You should also add test cases for your latest modifications.

dinesh.d updated this revision to Diff 9517.May 18 2014, 12:52 PM
dinesh.d edited edge metadata.

Thanks, I am finally able to recognize problem with the patch. I have missed to check if true/ false values are same as one of the operand in comp instruction.

Added 5 more test where my changes should not do anything. Y |= C case is combined by code following my changes in foldSelectICmpAndOr. These are kind of negative cases and if we add some transform for tested conditions, these test will fail. Do we keep this type of tests?

gentle ping.

bkramer accepted this revision.May 31 2014, 5:39 AM
bkramer edited edge metadata.

Looking good now. Thanks.

This revision is now accepted and ready to land.May 31 2014, 5:39 AM
dinesh.d closed this revision.Jun 2 2014, 12:32 AM
dinesh.d updated this revision to Diff 10006.

Closed by commit rL210006 (authored by dinesh).