[PATCH] InstCombine Check if Signed Addition Overflows

[Rafael Espíndola]

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

+    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?

+      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?


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

+        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?

Cheers,
Rafael




Cheers,
Rafael