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