[llvm] r209746 - InstCombine: Improvement to check if signed addition overflows.

[Rafael Espíndola]

WillNotOverflowSignedAdd should return the correct result for any LHS
and RHS. I agree that you found a case where it does not.  I think the
issues is that

 if (ComputeNumSignBits(LHS) > 1 && ComputeNumSignBits(RHS) > 1)
    return true;

Also needs to check that the sign bit is the same, not just hat LHS
and RHS have more then one sign bit.  Checking if that is the root
issue.




On 2 June 2014 16:52, suyog sarda <sardask01 at gmail.com> wrote:
>
>
>
> On Tue, Jun 3, 2014 at 2:08 AM, suyog sarda <sardask01 at gmail.com> wrote:
>>
>>
>>
>>
>> On Mon, Jun 2, 2014 at 8:17 PM, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>>
>>> BTW, I was curious why the tests looked more complicated than needed. It
>>> turns out we were not just adding nsw based on WillNotOverflowSignedAdd. I
>>> changed that in r210029. It should allow you to simplify the tests further.
>>>
>>
>> I am afraid if this is correct. Its adding 'nsw' to all additions, even
>> where it is sure not to add 'nsw'.
>>
>> ex:-
>>
>> int foo(short x){
>> short v =  (x & ~4) ;
>> int u = (int)v + 8;
>> return u;
>> }
>>
>> In this example we cannot determine if the addition will overflow of not,
>> because, one of the operand (x & ~4) has 0 bit at position 2 from LSB (LSB
>> position considered 0) while other operand 8 has 1 bit at position 3, which
>> is at higher significant position than position of 0. In this case its
>> giving IR as 'add nsw' which it should not.
>>
>
> Hi Rafael,
>
> More info on above test case with your patch.
>
> ~$ llvm/llvm/build/bin/clang -O0 -S -emit-llvm 1.c -fwrapv
> ~$ llvm/llvm/build/bin/opt -S -mem2reg 1.ll -o 2.ll
> ~$ cat 2.ll
>
> ; ModuleID = '1.ll'
> target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
> target triple = "i386-pc-linux-gnu"
>
> ; Function Attrs: nounwind
> define i32 @foo(i16 signext %x) #0 {
>
>   %1 = sext i16 %x to i32
>   %2 = and i32 %1, -5
>   %3 = trunc i32 %2 to i16
>   %4 = sext i16 %3 to i32
>   %5 = add i32 %4, 8
>   ret i32 %5
> }
>
> ~$ llvm/llvm/build/bin/opt -S -instcombine 2.ll
>
> ; ModuleID = '2.ll'
> target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
> target triple = "i386-pc-linux-gnu"
>
> ; Function Attrs: nounwind
> define i32 @foo(i16 signext %x) #0 {
>   %1 = and i16 %x, -5
>   %2 = sext i16 %1 to i32
>   %3 = add nsw i32 %2, 8
>   ret i32 %3
> }
>
> As we can see, add is being replaced by 'add nsw', which it should not.
> Thanks !
>
>
> With regards,
> Suyog Sarda