[PATCH] PR19958 wrong code at -O1 and above on x86_64-linux-gnu (InstCombine)

[Duncan P. N. Exon Smith]

> On 2014-Jul-07, at 11:37, suyog <suyog.sarda at samsung.com> wrote:
> 
> Hi Duncan, Nick, Rafael, Sanjay,
> 
> I have modified the code to calculate shift by Log2(Const2) - Log2(Const1) 
> instead of Log2(Const2/Const1) to avoid expensive division operation as per Duncan's suggestion.
> Also handled few more conditions for 0.
> Added additional test cases as well.
> 
> (I am doing this for icmp eq/ne only for now, 
> other icmp instructions are TODO item as they need some verification)
> 
> Can you please help in reviewing this modified patch.
> 
> Your suggestions/comments are most awaited. :)
> 
> Thanks,
> Suyog
> 
> http://reviews.llvm.org/D4068
> 
> Files:
>  lib/Transforms/InstCombine/InstCombineCompares.cpp
>  test/Transforms/InstCombine/icmp.ll

Hi Suyog,

Thanks for continuing to work on this.

I saw your reply about certain simplifications being handled elsewhere,
but I'm finding the assumptions in the code difficult to follow.

Firstly, I think you should handle every case locally.  There are two
ways to do this: with `assert`s for cases you can guarantee are handled
elsewhere, and with control flow for everything else.  IMO, all the
cases are so straightforward that there's no downside to just repeating
the simplification work.

As is, you're relying on transformations elsewhere to make this code
correct.  It's nearly impossible to confirm that the transformation is
logically sound, and what happens if someone removes (or disables) the
other transformation(s)?

Secondly, you're handling both `lshr` and `ashr`, which I think you can
do if you add the right logic -- but you're handling them identically.
This seems unlikely to be correct.

Thirdly, it looks like you're matching both `exact` and non-`exact`
cases.  This is possible -- with `exact`, the result is a poison value,
so you have some freedom -- but your comments only talk about `exact`,
and it's not clear that the code here is correct otherwise.

I have some more specific comments below.

Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -2459,6 +2459,48 @@
       return new ICmpInst(I.getPredicate(), A, B);
     }
 
+    // PR19753:
+    // (icmp eq/ne (ashr/lshr exact const2, A), const1) -> (icmp eq/ne A,
+    // Log2(const2/const1)) -> (icmp eq/ne A, Log2(const2) - Log2(const1)).

This would read better like this:

    // (icmp eq/ne (ashr/lshr exact const2, A), const1) ->
    // (icmp eq/ne A, Log2(const2/const1)) ->
    // (icmp eq/ne A, Log2(const2) - Log2(const1)).

+    // TODO : Handle this for other icmp instructions.
+    if (I.isEquality()) {
+      ConstantInt *CI2;
+      if (match(Op0, m_AShr(m_ConstantInt(CI2), m_Value(A))) ||
+          match(Op0, m_LShr(m_ConstantInt(CI2), m_Value(A)))) {

Shouldn't this be checking for `exact`?

+        APInt AP1 = CI->getValue();
+        APInt AP2 = CI2->getValue();
+        int Shift;
+        // (icmp eq/ne (ashr/lshr exact const2, A), 0) ->
+        // (icmp ugt A, Log2(const2)).
+        // const2 = 0/ both const = 0 is already handled previously.

Is this guaranteed, even if `-instcombine` is run on its own?  Is that
simplification guaranteed to run before this one?  If you're sure you
can guarantee this, you should use an `assert`:

    assert(AP2 && "Shift by 0 should be simplified by now");

If not, you should handle it explicitly.

+        if (!AP1) {
+          Shift = AP2.logBase2();
+          return new ICmpInst(I.ICMP_UGT, A,
+                              ConstantInt::get(A->getType(), Shift));
+        }
+        // if const1 = const2 -> icmp eq/ne A, 0
+        if (CI == CI2)
+          return new ICmpInst(I.getPredicate(), A,
+                              ConstantInt::getNullValue(A->getType()));
+        // If both the constants are negative, take their positive to calculate
+        // log.
+        if (AP1.isNegative() || AP2.isNegative()) {
+          AP1 = -AP1;
+          AP2 = -AP2;
+        }

This logic reads strangely, and I think it's wrong for `lshr`.

If you can guarantee that they are both negative or neither negative,
then this might be reasonable:

    if (isa<AShrOperator>(Op0) {
      assert(AP1.isNegative() == AP2.isNegative() &&
             "Different signs should be simplified by now");
      if (AP1.isNegative()) {
        AP1 = -AP1;
        AP2 = -AP2;
      }
    }

If you can't guarantee it, you should handle it explicitly instead.

+        Shift = AP2.logBase2() - AP1.logBase2();
+        if ((cast<BinaryOperator>(Op0)->isExact()) && (AP1.shl(Shift) == AP2))
+          return new ICmpInst(I.getPredicate(), A,
+                              ConstantInt::get(A->getType(), Shift));
+        // Use lshr here, since we've canonicalized to +ve numbers.
+        else if (AP1 == AP2.lshr(Shift))

Remove the `else`.  This should simply be: 

    if (AP1 == AP2.lshr(Shift))

+          return new ICmpInst(I.getPredicate(), A,
+                              ConstantInt::get(A->getType(), Shift));
+        else

Remove the `else` and un-indent the following `return`.

+          return ReplaceInstUsesWith(I, ConstantInt::getFalse(I.getType()));
+      }
+    }
+
     // If we have an icmp le or icmp ge instruction, turn it into the
     // appropriate icmp lt or icmp gt instruction.  This allows us to rely on
     // them being folded in the code below.  The SimplifyICmpInst code has