[PATCH] InstCombine Check if Signed Addition Overflows

[Jingyue Wu]

>
> 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).


I understand we are only considering the 1-set-bit case. However,
RHSKnownOne.countPopulation() == 1 doesn't mean there is only one 1 set bit
because there can be unknown ones as well. This is why I suggested to check
there are BitWidth-1 known zeros.



On Mon, May 26, 2014 at 5:45 AM, suyog sarda <sardask01 at gmail.com> wrote:

> Hi Rafael,
>
> Thanks for the review. Attaching updated patch after making changes
> according to suggestions.
>
> On Fri, May 23, 2014 at 1:04 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> > Hi,
>> >
>> > Attaching patch for checking if Signed Addition overflows.
>> > This patch implements two things :
>> >
>> > 1. If we know one number is positive and another is negative, we return
>> true
>> > as signed addition of two opposite signed numbers will never overflow.
>> >
>> > 2. Implemented TODO : If one of the operands only has one non-zero bit,
>> and
>> > if the other operand has a known-zero bit in a more significant place
>> than
>> > it (not including the sign bit) the ripple may go up to and fill the
>> zero,
>> > but won't change the sign.
>> > e.x -  (x & ~4) + 1
>> > We make sure that we are ignoring 0 at MSB.
>> >
>> > 3 Test case included :
>> >
>> > 1. To check if 0 in one number is at more significant place than 1 in
>> second
>> > number. We emit 'add nsw' as we are sure there will be no overflow as
>> the
>> > ripple will die at that zero.
>> > 2. If 0 is at less significant place than one, then emit normal 'add'
>> as we
>> > are not sure if the ripple will die before reaching Sign bit.
>> > 3. IF the two numbers are of opposite sign, we emit 'add nsw' as we are
>> sure
>> > that it will never overflow.
>>
>> Looks like an interesting optimization.
>>
>>   if (IntegerType *IT = cast<IntegerType>(LHS->getType())) {
>>
>> This function is always called with integer or vector of integer
>> types, right? I don't think it does a very good job for vectors. Can
>> you add a fixme about it? It should probably just check for vectors
>> upfront and check each element independently.
>>
>
> Added a TODO to handle Vectors.
>
>
>>
>> +    if (ConstantInt *CI = dyn_cast<ConstantInt>(RHS)) {
>>
>> CI is not used, so you can use a isa instead of a dyn_cast. But why do
>> you care if it is a constant? Can't you just remove the if?
>>
>
> 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.
>
>
>>
>> +      computeKnownBits(LHS, LHSKnownZero, LHSKnownOne);
>>
>> This is not a cheap function and ComputeSignBit uses it. Maybe just
>> move this call up and use the result both in here and in the above
>> sign bit comparison?
>>
>
> Corrected the code. Didn't use ComputeSignBits, instead used KnownBits
> result to determine sign bit.
>
>
>>
>>
>> +      if (RHSKnownOne.countPopulation() == 1) {
>>
>> This is symmetrical, so you should also handle the case where it is
>> the LHS that has only one bit set.
>>
>
> 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.
>
>
>>
>> +        int LHSZeroPosition = BitWidth -
>> LHSKnownZero.countLeadingZeros() - 1;
>>
>> This will be more conservative than needed if LHS is know to have a
>> zero bit before and one after the rhs 1 bit, no?
>>
>
> This i will try to implement in more generic way as discussed above. Can
> we go ahead for now?
> Hi Jingyue,
>
>>
>> +      if (RHSKnownOne.countPopulation() == 1) {
>>
>> This looks too aggressive. Are you supposed to say there are (BitWidth -
>> 1) known zeros?
>>
>
> 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).
>
>
>> Do you also need to check this single one-bit is not the sign bit or is
>> it implied by some later conditions?
>> +        // Ignore Sign Bit.
>>
>> 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.
> This was mentioned in TODO which i have implemented :)
>
> Thanks all for your valuable comments. Please help in reviewing the
> modified patch.
>
>
> --
> With regards,
> Suyog Sarda
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140526/f3edcde2/attachment.html>