This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold comparison of adding two zero extended booleans
Needs ReviewPublic

Authored by elhewaty on Sep 6 2023, 6:28 AM.

Details

Summary

zext i1 X + zext i1 Y == 0 --> !(or i1 X, Y)
zext i1 X + zext i1 Y == 1 --> xor i1 X, Y
zext i1 X + zext i1 Y == 2 --> and i1 X, Y

sext i1 X + sext i1 Y == 0 --> !(or i1 X, Y)
sext i1 X + sext i1 Y == 1 --> false
sext i1 X + sext i1 Y == 2 --> false

(sext i1 X + zext i1 Y) == 0 --> !(xor i1 X, Y)
(sext i1 X + zext i1 Y) == 1 --> (!X) & Y
(sext i1 X + zext i1 Y) == 2 --> false

zext i1 X + zext i1 Y != 0 --> or i1 X, Y
zext i1 Op0 + zext i1 Op1 != 1 --> !(xor i1 Op0, Op1)
zext i1 Op0 + zext i1 Op1 != 2 --> !(and i1 Op0, Op1)

sext i1 Op0 + sext i1 Op1 != 0 --> or i1 Op0, Op1
sext i1 Op0 + sext i1 Op1 != 1 --> true
sext i1 Op0 + sext i1 Op1 != 2 --> true

sext i1 Op0 + zext i1 Op1 != 0 --> xor i1 Op0, Op1
sext i1 Op0 + zext i1 Op1 != 1 --> Op0 | (!Op1)
sext i1 Op0 + zext i1 Op1 != 2 --> true

Diff Detail

Event Timeline

elhewaty created this revision.Sep 6 2023, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2023, 6:28 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
elhewaty requested review of this revision.Sep 6 2023, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2023, 6:28 AM
elhewaty retitled this revision from Fold comparison of add of zero extended booleans to [InstCombine] Fold comparison of add of zero extended booleans.Sep 6 2023, 7:14 AM
elhewaty added a reviewer: RKSimon.
elhewaty retitled this revision from [InstCombine] Fold comparison of add of zero extended booleans to [InstCombine] Fold comparison of adding two zero extended booleans.Sep 6 2023, 7:22 AM
elhewaty updated this revision to Diff 556044.Sep 6 2023, 8:12 AM
goldstein.w.n added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2905

Add (zext i1 X + zext i1 Y) == 2 --> and i1 X, Y?

2906

Can also do:

(sext i1 X + sext i1 Y) == 0 --> !(or i1 X, Y)
(sext i1 X + sext i1 Y) == 1 --> false
(sext i1 X + sext i1 Y) == 2 --> false

(sext i1 X + zext i1 Y) == 0 --> (xor i1 X, Y)
(sext i1 X + zext i1 Y) == 1 --> Y
(sext i1 X + zext i1 Y) == 2 --> false

2909

Somewhat nit, but I'm not a fan of using X and Y here. While it fallsthrough to nullptr now, somewhat may change that not realizing its necessary for correctness as X and Y may be overwritten.

Can you change to maybe Xi1 and Yi1 or really any temporary name you want.

2919

nit: No need for braces. Personally would do:

Case 1:
if(C.isOne())
  Cond = Builder.CreateXor(...)
Case 2:
else
   Cond = Builder.CreateNot(...)
...
arsenm added inline comments.Sep 6 2023, 11:18 AM
llvm/test/Transforms/InstCombine/icmp-add.ll
17

Don’t need the flags

36

Add tests showing the multiple use rejections

elhewaty updated this revision to Diff 556120.Sep 7 2023, 3:02 AM
elhewaty edited the summary of this revision. (Show Details)

Can you add alive2 links?

goldstein.w.n added inline comments.Sep 7 2023, 9:51 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2946

You can hoist this for all checks.

2956

Think would be a bit cleaner to do the following:

if(ICmp.IsEquality() && match(X, m_ZExtOrSExt(Op0)) && match(Y, m_ZExtOrSExt(Op1)) && Op0->getType->isI1() && Op1 ->getType()->isI1()) {
 if(match(X, m_Zext()) && match(Y, m_Zext()) {
...
}
if(match(X, m_SExt()) && match(Y, m_SExt()) {
...
}
if(match(X, m_ZExt()) && match(Y, m_Sext()) {
   swap(X, Y);
}
if(match(X, m_Sext() && match(Y, m_Zext()) {
....
}
}
elhewaty marked 5 inline comments as done.Sep 7 2023, 11:53 AM

@goldstein.w.n

// zext i1 X + zext i1 Y == 0 --> !(or i1 X, Y):
https://alive2.llvm.org/ce/z/Wfs9BS

// sext i1 X + sext i1 Y == 0 --> !(or i1 X, Y):
https://alive2.llvm.org/ce/z/ns3oDR

// zext i1 X + zext i1 Y == 1 --> xor i1 X, Y
https://alive2.llvm.org/ce/z/NvMYj9

// zext i1 X + zext i1 Y == 1 --> and i1 X, Y
https://alive2.llvm.org/ce/z/vZGCgR

// sext i1 X + sext i1 Y == 1 --> false
https://alive2.llvm.org/ce/z/UAg6BB

// sext i1 X + sext i1 Y == 2 --> false
https://alive2.llvm.org/ce/z/89xqHQ

// sext i1 X + zext i1 Y == 0 --> !(xor i1 X, Y)
https://alive2.llvm.org/ce/z/jQAkrJ

// sext i1 X + zext i1 Y == 1 --> !X & Y
https://alive2.llvm.org/ce/z/qYfXq5

// sext i1 X + zext i1 Y == 2 --> false
https://alive2.llvm.org/ce/z/cvw9VW

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2906

(sext i1 X + zext i1 Y) == 0 --> (xor i1 X, Y)

Is this transformation correct?

https://alive2.llvm.org/ce/z/vS_fMY

I think you meant sext i1 X + zext i1 Y == 0 --> !(xor i1 X, Y)

2906

(sext i1 X + zext i1 Y) == 1 --> Y

i think this also incorrect
what about (!X) & Y

llvm/test/Transforms/InstCombine/icmp-add.ll
36

Can you please explain what do you mean by multiple use rejections?

arsenm added inline comments.Sep 7 2023, 12:08 PM
llvm/test/Transforms/InstCombine/icmp-add.ll
36

You check (m_OneUse, so should have some negative tests where there is an additional use (e.g a store of the intermediate value). Similarly you don’t have negative tests for other compare types. You could handle ne/lt/ge but should at least have tests for them even if left unhandled

elhewaty updated this revision to Diff 556212.Sep 7 2023, 4:35 PM
elhewaty marked an inline comment as done.
elhewaty edited the summary of this revision. (Show Details)
goldstein.w.n added inline comments.Sep 7 2023, 4:53 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2912

You can do here or add TODO for exanding to arbtirary C
For the most part all values but a handful are just constant simplifications.
For sext -1 and -2 are basically parallels to the zext only case.

llvm/test/Transforms/InstCombine/icmp-add.ll
133

Missing test for icmp ne (which I think you will incorrectly simplify as false when it should be true).

elhewaty updated this revision to Diff 556246.Sep 8 2023, 4:03 AM
elhewaty marked 2 inline comments as done.
elhewaty edited the summary of this revision. (Show Details)

// zext i1 X + zext i1 Y != 0 --> or i1 X, Y
https://alive2.llvm.org/ce/z/Rg3mm7

// zext i1 Op0 + zext i1 Op1 != 1 --> !(xor i1 Op0, Op1)
https://alive2.llvm.org/ce/z/WFyF_2

// zext i1 Op0 + zext i1 Op1 != 2 --> !(and i1 Op0, Op1)
https://alive2.llvm.org/ce/z/-QgaLR

// sext i1 Op0 + sext i1 Op1 != 0 --> or i1 Op0, Op1
https://alive2.llvm.org/ce/z/oPXhLY

// sext i1 Op0 + sext i1 Op1 != 1 --> true
https://alive2.llvm.org/ce/z/xTWVYh

// sext i1 Op0 + sext i1 Op1 != 2 --> true
https://alive2.llvm.org/ce/z/GoroRZ

// sext i1 Op0 + zext i1 Op1 != 0 --> xor i1 Op0, Op1
https://alive2.llvm.org/ce/z/mN7g3V

// sext i1 Op0 + zext i1 Op1 != 1 --> Op0 | (!Op1)
https://alive2.llvm.org/ce/z/FT_8iD

// sext i1 Op0 + zext i1 Op1 != 2 --> true
https://alive2.llvm.org/ce/z/ih8K7-

llvm/test/Transforms/InstCombine/icmp-add.ll
36

Okay, I will do my best.
But I am just waiting for the code to be fully accepted, then I will try to write tests.

arsenm added inline comments.Sep 8 2023, 4:21 AM
llvm/test/Transforms/InstCombine/icmp-add.ll
36

This is backwards. It’s easier to review a patch that has full tests preconmitted so the patch diff shows the effects

elhewaty updated this revision to Diff 556275.Sep 8 2023, 9:19 AM
elhewaty marked an inline comment as done.
elhewaty marked 4 inline comments as done.Sep 14 2023, 9:54 AM

Can any one review this patch

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2912

I did and added alive2 tests. Please file a bug and assign me to it.
I will be very busy in the next few weeks. If the patch is good at this status, please land it.

llvm/test/Transforms/InstCombine/icmp-add.ll
36

so we need tests for these cases:
zext/zext lt
zext/zext gt

sext/sext lt/gt
sext/zext lt/gt

zext/zext oneuse
sext/zext oneuse
sext/onesext oneuse

133

I added tests and handled these cases in the code