[PATCH] D62818: [InstCombine] Fix fold order for icmp pred (and (sh X, Y), C), 0.

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz marked 5 inline comments as done.
huihuiz added a comment.

Thank you so much for all the review feedback, really appreciate it! :)



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1660
+    // ((X & ~7) == 0) --> X < 8
+    if ((~(*C2) + 1).isPowerOf2()) {
+      Constant *NegBOC =
----------------
lebedev.ri wrote:
> `C2->negate().isPowerOf2()`
Should not call C2->negate()
If C2 negate is not power of 2, then calling negate() will replace C2 with C2 negate.
C2 should not be modified.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2791-2792
-
-      // Replace (and X, (1 << size(X)-1) != 0) with x s< 0
-      if (BOC->isSignMask()) {
-        Constant *Zero = Constant::getNullValue(BOp0->getType());
----------------
lebedev.ri wrote:
> Uhm, where did this check that we were comparing with `0`?
What happened was, C being 0, signbit, other number

1. we are ok with 0

2. if C is signbit, consider test: X & signbit == signbit
fold:
X & -C == -C -> X >  u ~C
X & -C != -C -> X <= u ~C
and fold:
For i32: x >u 2147483647 -> x <s 0  -> true if sign bit set

are scheduled before
fold: (and X, (1 << size(X)-1) != 0) with x s< 0

3. if C is other number, SimplifyICmpInst will do its job 






================
Comment at: test/Transforms/InstCombine/pr17827.ll:66
 
 ; Unsigned compare allows a transformation to compare against 0.
 define i1 @test_shift_and_cmp_changed2(i8 %p) {
----------------
lebedev.ri wrote:
> These don't look like improvements to me.
> Looks like that reordering exposes yet another missing fold.
in D63026
I am moving fold  ((X & ~7) == 0) --> X < 8 ahead.

If X is (BinOp Y, C3), should allow other rules to fold C3 with C2, 
eg (X >> C3) & C2 != C1  ->  (X & (C2 << C3)) != (C1 << C3)



Repository:
  rL LLVM

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

https://reviews.llvm.org/D62818