[llvm-commits] CVS: llvm/lib/Transforms/Scalar/InstructionCombining.cpp

[Chris Lattner]

On Mar 25, 2007, at 12:10 PM, Reid Spencer wrote:

> On Sun, 2007-03-25 at 11:55 -0700, Chris Lattner wrote:
>>> Index: llvm/lib/Transforms/Scalar/InstructionCombining.cpp
>>> diff -u llvm/lib/Transforms/Scalar/InstructionCombining.cpp:1.687
>>> llvm/lib/Transforms/Scalar/InstructionCombining.cpp:1.688
>>> --- llvm/lib/Transforms/Scalar/InstructionCombining.cpp:1.687	Sun
>>> Mar 25 00:01:29 2007
>>> +++ llvm/lib/Transforms/Scalar/InstructionCombining.cpp	Sun Mar 25
>>> 00:33:51 2007
>>> @@ -540,8 +540,9 @@
>>>        if (I->getOpcode() == Instruction::Shl)
>>>          if ((CST = dyn_cast<ConstantInt>(I->getOperand(1)))) {
>>>            // The multiplier is really 1 << CST.
>>> -          Constant *One = ConstantInt::get(V->getType(), 1);
>>> -          CST = cast<ConstantInt>(ConstantExpr::getShl(One, CST));
>>> +          APInt Multiplier(V->getType()->getPrimitiveSizeInBits(),
>>> 0);
>>> +          Multiplier.set(CST->getZExtValue()); // set bit is == 1
>>> << CST
>>
>> This doesn't seem safe.  Won't getZextValue assert if CST is > 64  
>> bits?
>
> The same comment you made about huge shift values applies here. I was
> assuming the shift amount would always fit in 64-bits (32, really),  
> but
> as you mentioned before, it could be some huge value. Will fix.  In
> practice, this is quite unlikely to cause a problem.

Right.

>> Also, I don't understand the gymnastics you're doing with Multiplier.
>
> Just trying to get rid of an expensive ConstantExpr::getShl. In  
> addition
> to the ConstantExpr overhead, the resulting Shl on an APInt isn't  
> super
> cheap, even for the <= 64 bits case. Setting a bit is pretty cheap.  
> But,
> I'll probably just revert to get over the "huge value" issue.

k

>
>>> +/// SubOne - Subtract one from a ConstantInt
>>>  static ConstantInt *SubOne(ConstantInt *C) {
>>
>> Shouldn't these use ++/-- on APInt?  That seems more efficient.
>
> I should really have these functions declare the parameter  
> constant. We
> don't want to increment the APInt inside C. Either way, a copy of the
> value is being made.

Right.  ConstantInt's are immutable, so it doesn't really need to be  
marked const.  I'm saying that the implementation of these methods  
shouldn't build a "1" apint, then add it.  Instead, it should just  
increment an apint with ++.


>>
>>> @@ -2188,14 +2203,12 @@
>>>
>>>    ConstantInt *C1;
>>>    if (Value *X = dyn_castFoldableMul(Op0, C1)) {
>>> -    if (X == Op1) { // X*C - X --> X * (C-1)
>>> -      Constant *CP1 = ConstantExpr::getSub(C1, ConstantInt::get
>>> (I.getType(),1));
>>> -      return BinaryOperator::createMul(Op1, CP1);
>>> -    }
>>> +    if (X == Op1)  // X*C - X --> X * (C-1)
>>> +      return BinaryOperator::createMul(Op1, SubOne(C1));
>>
>> I like this set of changes, much cleaner.
>
> This was the real intent .. makes the code more readable and a little
> bit faster too.

Yep, thanks again,

-Chris