[llvm] r185272 - ValueTracking: Teach isKnownToBeAPowerOfTwo about (ADD X, (XOR X, Y)) where X is a power of two

[David Majnemer]

On Jul 9, 2013, at 5:34 AM, Jay Foad <jay.foad at gmail.com> wrote:

> On 30 June 2013 00:44, David Majnemer <david.majnemer at gmail.com> wrote:
>> Author: majnemer
>> Date: Sat Jun 29 18:44:53 2013
>> New Revision: 185272
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=185272&view=rev
>> Log:
>> ValueTracking: Teach isKnownToBeAPowerOfTwo about (ADD X, (XOR X, Y)) where X is a power of two
>> 
>> This allows us to simplify urem instructions involving the add+xor to
>> turn into simpler math.
>> 
>> Modified:
>>    llvm/trunk/lib/Analysis/ValueTracking.cpp
>>    llvm/trunk/test/Transforms/InstCombine/rem.ll
>> 
>> Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=185272&r1=185271&r2=185272&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
>> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Sat Jun 29 18:44:53 2013
>> @@ -855,16 +855,24 @@ bool llvm::isKnownToBeAPowerOfTwo(Value
>>     return false;
>>   }
>> 
>> -  // Adding a power of two to the same power of two is a power of two or zero.
>> -  if (OrZero && match(V, m_Add(m_Value(X), m_Value(Y)))) {
>> -    if (match(X, m_And(m_Value(), m_Specific(Y)))) {
>> -      if (isKnownToBeAPowerOfTwo(Y, /*OrZero*/true, Depth))
>> -        return true;
>> -    } else if (match(Y, m_And(m_Value(), m_Specific(X)))) {
>> -      if (isKnownToBeAPowerOfTwo(X, /*OrZero*/true, Depth))
>> -        return true;
>> -    }
>> -  }
>> +  if (match(V, m_Add(m_Value(X), m_Value(Y))))
>> +    if (OverflowingBinaryOperator *VOBO = cast<OverflowingBinaryOperator>(V))
>> +      if (OrZero || VOBO->hasNoUnsignedWrap() || VOBO->hasNoSignedWrap()) {
>> +        // Adding a power of two to the same power of two is a power of two or
>> +        // zero.
>> +        if (BinaryOperator *XBO = dyn_cast<BinaryOperator>(X))
>> +          if (XBO->getOpcode() == Instruction::And ||
>> +              XBO->getOpcode() == Instruction::Xor)
>> +            if (XBO->getOperand(0) == Y || XBO->getOperand(1) == Y)
>> +              if (isKnownToBeAPowerOfTwo(Y, /*OrZero*/true, Depth))
>> +                return true;
>> +        if (BinaryOperator *YBO = dyn_cast<BinaryOperator>(Y))
>> +          if (YBO->getOpcode() == Instruction::And ||
>> +              YBO->getOpcode() == Instruction::Xor)
>> +            if (YBO->getOperand(0) == X || YBO->getOperand(1) == X)
>> +              if (isKnownToBeAPowerOfTwo(X, /*OrYero*/true, Depth))
>> +                return true;
>> +      }
>> 
>>   // An exact divide or right shift can only shift off zero bits, so the result
>>   // is a power of two only if the first operand is a power of two and not
> 
> The original code said that if X is a power of two or zero, then
> X+(X&Y) is a power of two or zero. OK.
> 
> The new code says that if X is a power of two or zero, then X+(X&Y)
> and X+(X^Y) are both powers of two (not zero). (I'm assuming the + is
> a normal C addition of signed integers, so it hasNoSignedWrap().)
> 
> This is clearly wrong if X is zero; 0+(0&99) is not a power of two.

It is not a power of two but it is zero. If X is zero then X+(X&Y) is zero.

> 
> It's also clearly wrong if Y has bits set that are not set in X;
> 4+(4^99) is not a power of two.
> 
> Or have I misread the code?

This looks like it is wrong. I'll fix it.

> 
> Jay.