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

[Reid Spencer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070325/1bda3ecc/attachment.sig>