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.
>
> 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.
>
> > + CST = ConstantInt::get(Multiplier);
> > return I->getOperand(0);
> > }
> > }
> > @@ -558,14 +559,31 @@
> > return false;
> > }
> >
> > +/// AddOne - Add one to a ConstantInt
> > static ConstantInt *AddOne(ConstantInt *C) {
> > + APInt One(C->getType()->getPrimitiveSizeInBits(),1);
> > + return ConstantInt::get(C->getValue() + One);
> > }
> > +/// 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.
>
> > @@ -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.
Reid.
>
> -Chris
signature.asc
Description: This is a digitally signed message part
_______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
