<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 2, 2014 at 8:17 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.</div>
<div class=""><div class="h5">
<div class="gmail_extra"><br></div></div></div></blockquote><div><br></div><div>I am afraid if this is correct. Its adding 'nsw' to all additions, even where it is sure not to add 'nsw'.<br><br></div><div>
ex:-<br> <i><br>int foo(short x){<br>short v =  (x & ~4) ;<br>int u = (int)v + 8;<br>return u;<br>}<br></i> <br></div></div>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.<br>
<br></div><div class="gmail_extra">I think the arguments being passed to 'WillNotOverflowSignedAdd' are wrong in your patch and hence such results.<br></div><div class="gmail_extra">In the previous call to 'WillNotOverflowSignedAdd', the arguments are not simple LHS/RHS but modified as highlighted in bold below:<br>
<i><br>if (<b>SExtInst *LHSConv = dyn_cast<SExtInst>(LHS)</b>) {<br>    // (add (sext x), cst) --> (sext (add x, cst'))<br>    if (ConstantInt *RHSC = dyn_cast<ConstantInt>(RHS)) {<br>      <b>Constant *CI =<br>
          ConstantExpr::getTrunc(RHSC, LHSConv->getOperand(0)->getType())</b>;<br>      if (LHSConv->hasOneUse() &&<br>          ConstantExpr::getSExt(CI, I.getType()) == RHSC &&<br>          <b>WillNotOverflowSignedAdd(LHSConv->getOperand(0), CI)) {</b><br>
        // Insert the new, smaller add.<br>        Value *NewAdd =<br>            Builder->CreateNSWAdd(LHSConv->getOperand(0), CI, "addconv");<br>        <b>if (!I.hasNoSignedWrap()) {<br>          Changed = true;<br>
          I.setHasNoSignedWrap(true);<br>        }</b><br>        return new SExtInst(NewAdd, I.getType());<br>      }<br>    }</i><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">
I think we should move the code to set SignedWrap flag as highlighted above.<br><br></div><div class="gmail_extra">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.<br>
<br></div><div class="gmail_extra">Please help in reviewing the patch as well as if my analysis is correct for your patch.<br><br></div><div class="gmail_extra">Thanks!<br></div><div class="gmail_extra"><br>-- <br>With regards,<br>
Suyog Sarda<br>
</div></div>