[llvm-commits] [llvm] r132316 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp test/Transforms/InstCombine/2011-05-28-swapmulsub.ll

[Duncan Sands]

Hi Stuart,

> @@ -135,6 +135,23 @@
>           return BinaryOperator::CreateAdd(Add, Builder->CreateMul(C1, CI));
>         }
>       }
> +
> +    // (1 - X) * (-2) ->  (x - 1) * 2, for all positive nonzero powers of 2

the big X becomes a little x later in the comment.  It is not clear that the
power of 2 comment is about 2 rather than about X.

> +    // The "* 2" thus becomes a potential shifting opportunity.
> +    {
> +      const APInt&    Val = CI->getValue();
> +      const APInt&PosVal = Val.abs();
> +      if (Val.isNegative()&&  PosVal.isPowerOf2()) {

You should also check that Op0, aka (1-X), has only one use.  Also, why does it
matter that it is "1-X" specifically?  Surely "Constant - X", "X - Constant",
"Constant+X" and "X-Constant" are also fine, since you don't increase the
amount of computation in any of these when you push the "*(-1)" into them?

> +        Value *X = 0;
> +        if (match(Op0, m_Sub(m_One(), m_Value(X)))) {
> +          // ConstantInt::get(Op0->getType(), 2);

No need for this commented out line...

Ciao, Duncan.