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

[suyog sarda]

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.

I think the arguments being passed to 'WillNotOverflowSignedAdd' are wrong
in your patch and hence such results.
In the previous call to 'WillNotOverflowSignedAdd', the arguments are not
simple LHS/RHS but modified as highlighted in bold below:


















*if (SExtInst *LHSConv = dyn_cast<SExtInst>(LHS)) {    // (add (sext x),
cst) --> (sext (add x, cst'))    if (ConstantInt *RHSC =
dyn_cast<ConstantInt>(RHS)) {      Constant *CI =
ConstantExpr::getTrunc(RHSC, LHSConv->getOperand(0)->getType());      if
(LHSConv->hasOneUse() &&          ConstantExpr::getSExt(CI, I.getType()) ==
RHSC &&          WillNotOverflowSignedAdd(LHSConv->getOperand(0), CI))
{        // Insert the new, smaller add.        Value *NewAdd =
Builder->CreateNSWAdd(LHSConv->getOperand(0), CI, "addconv");        if
(!I.hasNoSignedWrap()) {          Changed = true;
I.setHasNoSignedWrap(true);        }        return new SExtInst(NewAdd,
I.getType());      }    }*


I think we should move the code to set SignedWrap flag as highlighted above.

Attaching updated patch to handle case where we have operands with
bitwidth=1 and also moved the code to set SignedWrap flag inside as shown
above. Also added test case for handling operands with bitwidth=1.

Please help in reviewing the patch as well as if my analysis is correct for
your patch.

Thanks!

-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/fce73192/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AddOverFlow_3.patch
Type: text/x-patch
Size: 9346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/fce73192/attachment.bin>