[PATCH] InstCombine Check if Signed Addition Overflows

[Jingyue Wu]

LGTM

-  // will sign extend fine.
+  // will sign extend fine.s

typo?




On Tue, May 27, 2014 at 10:31 AM, suyog sarda <sardask01 at gmail.com> wrote:

>
>
>
> On Tue, May 27, 2014 at 7:29 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> Sorry for the delay, I was stuck in the GlobalAlias patches.
>>
>> Please be sure to address Jingyue Wu comment too.
>>
>>
> Oops !! Got the point. Thanks Jingyue Wu for pointing out. Now i am doing
> this by
> ((~RHSKnownZero).countpopulation == 1) which combines (
> RHSKnownOne.countpopulation == 1 && RHSKnownZero.countpopulation ==
> BitWidth-1 ). This will ensure that only one bit is 1 and all other bits
> are 0.
> Also added a test case related to this e.g (x & ~4) + (x | 2) -> this we
> are not sure if overflow doesn't occur
> as we are not sure of other bits in (x | 2) -> so 'add' will not convert
> to 'add nsw'.
>
>
>> +    APInt LHSKnownZero(BitWidth, 0, 1);
>
>
>> Since the value is 0, there is no difference in passing isSigned=true.
>> If you think that is better to be explicit, please write it as
>>
>> APInt LHSKnownZero(BitWidth, 0, /*isSigned*/ true);
>>
>
> Made this change.
>
>
>>
>> +    // TODO: Handle for Vectors.
>>
>> The comment is more for the entire function, no just this particular
>> case. Please move it to the top.
>>
>>
> Done !
>
>
>> +  if (RHSKnownOne.countPopulation() == 1)
>>
>> Jingyue Wu comment....
>>
>
> Implemented.
>
>
>>
>> Please don't duplicate the code to handle the symmetry. You can
>> probably use a small static helper function and write
>>
>> if (helper(LHSKnowZero, LHSKnowOne, RHSKnownZero, RHSKnownOne))
>>   return true;
>>
>> if (helper(RHSKnownZero, RHSKnownOne, LHSKnowZero, LHSKnowOne))
>>   return true;
>>
>>
> Added a static helper function to do this.
>
>
>>  +      int LHSZeroPosition = BitWidth - LHSKnownZero.countLeadingZeros()
>> - 1;
>>
>> Please add a FIXME about this missing the case where LHS has a zero
>> before the 1 in the RHS, but also has one after.
>>
>>
> Added this FIXME.
>
>
>> Cheers,
>> Rafael
>>
>
> Thanks a lot Rafael, Jingyue Wu for helping out in this implementation.
> Attaching updated patch.
> Please see if this looks good.
>
> --
> With regards,
> Suyog Sarda
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140527/4addf4d9/attachment.html>