[PATCH] D159464: [InstCombine] Fold comparison of adding two zero extended booleans

[Mohamed Atef via Phabricator via llvm-commits]

elhewaty marked 4 inline comments as done.
elhewaty added a comment.

Can any one review this patch



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2912
+      Op1->getType()->isIntOrIntVectorTy(1) &&
+      (C.isZero() || C.isOne() || (C.exactLogBase2() == 1))) {
+    Value *Cond = Builder.getFalse();
----------------
goldstein.w.n wrote:
> 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.
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.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-add.ll:133
+  ret i1 %t
+}
+
----------------
goldstein.w.n wrote:
> Missing test for `icmp ne` (which I think you will incorrectly simplify as `false` when it should be `true`).
I added tests and handled these cases in the code


================
Comment at: llvm/test/Transforms/InstCombine/icmp-add.ll:36
+}
+
 define i1 @test1(i32 %a) {
----------------
arsenm wrote:
> elhewaty wrote:
> > arsenm wrote:
> > > elhewaty wrote:
> > > > arsenm wrote:
> > > > > Add tests showing the multiple use rejections
> > > > Can you please explain what do you mean by multiple use rejections? 
> > > 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 
> > 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.
> This is backwards. It’s easier to review a patch that has full tests preconmitted so the patch diff shows the effects 
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





CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159464/new/

https://reviews.llvm.org/D159464