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

[Jay Foad]

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

Jay.