<div dir="ltr"><div>Hi Rafael,<br><br></div>Thanks for the review. Attaching updated patch after making changes according to suggestions. <br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 23, 2014 at 1:04 AM, 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 class="">> Hi,<br>
><br>
> Attaching patch for checking if Signed Addition overflows.<br>
> This patch implements two things :<br>
><br>
> 1. If we know one number is positive and another is negative, we return true<br>
> as signed addition of two opposite signed numbers will never overflow.<br>
><br>
> 2. Implemented TODO : If one of the operands only has one non-zero bit, and<br>
> if the other operand has a known-zero bit in a more significant place than<br>
> it (not including the sign bit) the ripple may go up to and fill the zero,<br>
> but won't change the sign.<br>
> e.x - (x & ~4) + 1<br>
> We make sure that we are ignoring 0 at MSB.<br>
><br>
> 3 Test case included :<br>
><br>
> 1. To check if 0 in one number is at more significant place than 1 in second<br>
> number. We emit 'add nsw' as we are sure there will be no overflow as the<br>
> ripple will die at that zero.<br>
> 2. If 0 is at less significant place than one, then emit normal 'add' as we<br>
> are not sure if the ripple will die before reaching Sign bit.<br>
> 3. IF the two numbers are of opposite sign, we emit 'add nsw' as we are sure<br>
> that it will never overflow.<br>
<br>
</div>Looks like an interesting optimization.<br>
<br>
if (IntegerType *IT = cast<IntegerType>(LHS->getType())) {<br>
<br>
This function is always called with integer or vector of integer<br>
types, right? I don't think it does a very good job for vectors. Can<br>
you add a fixme about it? It should probably just check for vectors<br>
upfront and check each element independently.<br></blockquote><div><br></div><div>Added a TODO to handle Vectors. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+ if (ConstantInt *CI = dyn_cast<ConstantInt>(RHS)) {<br>
<br>
CI is not used, so you can use a isa instead of a dyn_cast. But why do<br>
you care if it is a constant? Can't you just remove the if?<br></blockquote><div><br></div><div>Removed this 'if' in the new patch. The patch for now works for an operand of which we know that it has only one Known 1 set bit. For multiple set bits in one of the operand, we are not sure till what length the ripple of carry bit will go on. We need to work on this logic. I will try to see if i can come up with more generalized logic. For now, this patch works atleast for example like ((x & ~4) + 1), ((x &~4) + 2), where one of the operand has only one set bit. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+ computeKnownBits(LHS, LHSKnownZero, LHSKnownOne);<br>
<br>
This is not a cheap function and ComputeSignBit uses it. Maybe just<br>
move this call up and use the result both in here and in the above<br>
sign bit comparison?<br></blockquote><div><br></div><div>Corrected the code. Didn't use ComputeSignBits, instead used KnownBits result to determine sign bit.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
+ if (RHSKnownOne.countPopulation() == 1) {<br>
<br>
This is symmetrical, so you should also handle the case where it is<br>
the LHS that has only one bit set.<br></blockquote><div><br></div><div>Added a symmetrical code. Earlier, i had considered one of the operand as constant, and as constant are arranged always to right, was considering only one part of the symmetry. Now i had removed 'if constant' and added the symmetric code.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+ int LHSZeroPosition = BitWidth - LHSKnownZero.countLeadingZeros() - 1;<br>
<br>
This will be more conservative than needed if LHS is know to have a<br>
zero bit before and one after the rhs 1 bit, no?<br></blockquote><div><br></div><div>This i will try to implement in more generic way as discussed above. Can we go ahead for now?<br><h3 class=""><span style="font-weight:normal"><span name="Jingyue Wu" class="">Hi Jingyue,</span></span></h3>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<div class=""><div>+ if (RHSKnownOne.countPopulation() == 1) { </div></div><div>This
looks too aggressive. Are you supposed to say there are (BitWidth - 1)
known zeros? </div></blockquote><div><br></div><div>For now we are considering that one of the operand has only 1 set bit. For multiple set bit, we need to come up with more generalized logic to check how much the ripple of carry bit of addition will go on and not affect the sign bit (i.e. it will not overflow).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>Do you also need to check this single one-bit is not the
sign bit or is it implied by some later conditions? </div>
<div>+ // Ignore Sign Bit.</div><div class="gmail_extra"><br></div></blockquote><div><div class="gmail_extra">We are ignoring the 0 at sign bit, because if ripple of carry bit of addition reaches sign bit, it may affect the sign of final result.<br>
</div><div class="gmail_extra">This was mentioned in TODO which i have implemented :) <br><br></div><div class="gmail_extra">Thanks all for your valuable comments. Please help in reviewing the modified patch.<br></div><br>
</div></div><br>-- <br>With regards,<br>Suyog Sarda<br>
</div></div>