[PATCH] InstCombine Check if Signed Addition Overflows

[suyog sarda]

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/ce264fb4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AddOverFlow.patch
Type: text/x-patch
Size: 3631 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140526/ce264fb4/attachment.bin>