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

[Stuart Hastings]

Agreed.  Followup patch at 132348.

Thank you both for the reviews,

stuart

On May 30, 2011, at 1:19 PM, Duncan Sands wrote:

> 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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits


> On Mon, May 30, 2011 at 10:19 PM, Duncan Sands <baldrick at free.fr> wrote:
>>> +    // 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?
> 
> You mentioned "X-Constant" twice, but you probably meant "X+Constant"
> the second time.
> 
> There's no need to match "Constant+X", since it will be turned into
> "X+Constant" by other parts of instcombine.
> 
> I also don't think that for the subtraction case you even need either
> one to be a constant at all. -(X-Y) == (Y-X) even for non-constant X
> and Y.
> 
> That leaves just the cases "(X-Y)*-PowerOfTwo" and
> "(X+Constant)*-PowerOfTwo" to consider.
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits