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

[suyog sarda]

Thanks Duncan for your quick reply :)


After looking at `SimplifyICmpInst`, the logic there seems disjoint

enough that I think it's better just to handle the cases redundantly
> here.  Even though it's called directly from this function, it's hard to
> verify exactly how its transformations overlap with these ones.  Unless
> you can show a non-negligible compile time improvement from switching to
> `assert`s, I think the clearer code wins.
>
> (It's a weak preference, so if anyone following along thinks `assert`
> is better here, pipe up!)
>

Seems better to include all the cases. I will update the patch accordingly.


>
> > 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.
> >
> > Since, the special cases for exact were handled in 'instsimplify' for
> lshr and ashr,
> > i combined the common logic. The only difference was checking the
> equality after
> > calculating the logs, which i have handled as separate cases.
> >
> >
> > 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)).
> >
> > Will rectify this :)
> >
> >
> > +    // 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`?
> >
> > The only difference for 'exact' and 'non-exact' was checking the
> equality after
> > calculating the logs, which i have handled as separate cases below.
> >
> >
> > +        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.
> >
> > As mentioned earlier, this is handled in 'Simplify'.
> > I will modify the code (assert or handle the case itself), as per you
> suggestion.
> >
> >
> > +        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.
> >
> > There should be '&&' instead of '||'. i will rectify it.
> >
> > For, both ashr/lshr  "exact" if the constants have opposite sign,
> > 'Simplify' returns false, and we never reach this place.
>
> I'm not convinced about `lshr exact`.  Consider:
>
>     %Shifted = lshr exact i2 2, %A
>     %Compare = icmp eq i2 %Shifted, 1
>
> Here, 2 is negative and 1 is positive, but the %Compare should be `i1 1`
> when %A is 1.
>
> If simplify gives `i1 0` for this, it sounds like a bug to me.  Can you
> confirm?
>

Strangely, the output varies here.

cat test.ll
define i1 @main(i2 %a) {
  %shr = lshr exact i2 2, %a
  %cmp = icmp eq i2 %shr, 1
  ret i1 %cmp
}

llvm/llvm/build/bin/opt -S -instcombine test.ll
; ModuleID = 'test.ll'

define i1 @main(i2 %a) {
  %shr = lshr exact i2 -2, %a
  %cmp = icmp eq i2 %shr, 1
  ret i1 %cmp
}

llvm/llvm/build/bin/opt -S -instsimplify test.ll
; ModuleID = 'test.ll'

define i1 @main(i2 %a) {
  %shr = lshr exact i2 -2, %a
  %cmp = icmp eq i2 %shr, 1
  ret i1 %cmp
}

As visible above, the 'Simplify' or 'Combine' doesn't return false.

However,
Consider following Test case :

cat test.ll
define i1 @main(i32 %a) {
  %shr = lshr exact i32 -64, %a // One positive one negative
  %cmp = icmp eq i32 %shr, 16
  ret i1 %cmp
}

llvm/llvm/build/bin/opt -S -instsimplify test.ll
; ModuleID = 'test.ll'

define i1 @main(i32 %a) {
  ret i1 false
}

llvm/llvm/build/bin/opt -S -instcombine test.ll
; ModuleID = 'test.ll'

define i1 @main(i32 %a) {
  ret i1 false
}

It seems, 'Simplify' is behaving differently for 'extreme (MIN/MAX)' values
and values in 'middle'. Is this logically correct (doesn't seem so)? Or a
bug?


>
> > For ashr non-'exact', if the constants have opposite sign, 'Simplify'
> > returns false.
> >
> > For lshr non-'exact', if const2 is positive and const1 is negative,
> > 'Simplify' returns false.
>
> Adding the logic (or `assert`s) will make this more obvious :).
>
> > For lshr non-'exact', if const2 is negative and const1 is positive,
> > we will have to handle this case. I missed this, will handle it.
> >
>



-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140708/44f34a48/attachment.html>