[llvm-commits] CVS: llvm/lib/VMCore/ConstantFold.cpp

[Reid Spencer]

On Tue, 2007-02-27 at 09:08 -0800, Chris Lattner wrote:
> On Feb 26, 2007, at 10:24 PM, Reid Spencer wrote:
> 
> >    case Instruction::FPToUI:
> > -    if (const ConstantFP *FPC = dyn_cast<ConstantFP>(V))
> > -      return ConstantInt::get(DestTy,(uint64_t) FPC->getValue());
> > +    if (const ConstantFP *FPC = dyn_cast<ConstantFP>(V)) {
> > +      APInt Val(APIntOps::RoundDoubleToAPInt(FPC->getValue()));
> > +      uint32_t DestBitWidth = cast<IntegerType>(DestTy)- 
> > >getBitWidth();
> > +      if (Val.getBitWidth() > DestBitWidth)
> > +        Val.trunc(DestBitWidth);
> 
> This seems like a poor choice of API.  RoundDoubleToAPInt should take  
> a result bitwidth.  Without this, this appears like it could make a  
> "large" APInt, then truncate it down to a small one when converting  
> from double, which is very inefficient.

Done.

> 
> > @@ -191,23 +206,37 @@
> >      return 0;                   // Other pointer types cannot be  
> > casted
> >    case Instruction::UIToFP:
> >      if (const ConstantInt *CI = dyn_cast<ConstantInt>(V))
> > -      return ConstantFP::get(DestTy, double(CI->getZExtValue()));
> > +      if (CI->getType()->getBitWidth() <= APInt::APINT_BITS_PER_WORD)
> > +        return ConstantFP::get(DestTy, CI->getValue().roundToDouble 
> > (false));
> 
> Why the check for APINT_BITS_PER_WORD here?  APINT_BITS_PER_WORD is  
> an implementation detail of APInt, not a part of its public  
> interface.  

Actually it is part of the public interface and it needs to be. For
example, the constructor that takes an array of words (as might be used
by bcreader) needs to know how many bits are in those words. This
constant tells you.


> Also, please split the roundToDouble method into two  
> methods which don't take a bool.

I don't understand this comment at all. What should these methods do
differently?

> 
> >      return 0;
> >    case Instruction::SIToFP:
> >      if (const ConstantInt *CI = dyn_cast<ConstantInt>(V))
> > -      return ConstantFP::get(DestTy, double(CI->getSExtValue()));
> > +      if (CI->getType()->getBitWidth() <= APInt::APINT_BITS_PER_WORD)
> > +        return ConstantFP::get(DestTy, CI->getValue().roundToDouble 
> > (true));
> >      return 0;
> 
> Likewise.
> 
> >
> > @@ -382,7 +412,7 @@
> >      Ops.reserve(numOps);
> >      for (unsigned i = 0; i < numOps; ++i) {
> >        const Constant *Op =
> > -        (i == idxVal) ? Elt : Constant::getNullValue(Elt->getType());
> > +        (idxVal == i) ? Elt : Constant::getNullValue(Elt->getType());
> 
> This sort of change makes the diff more noisy, and there is no  
> apparent reason for doing it.  Generally, the mutating value (i) is  
> on the LHS, why do this (in many places)?

Becasue there is no operator==(int, APInt) but there is an
APInt::operator==(int).

> 
> 
> > @@ -554,56 +585,66 @@
> >        case Instruction::SDiv:
> > +        if (CI2->isNullValue())
> > +          return 0;        // X / 0 -> can't fold
> > +        return ConstantInt::get(C1->getType(), C1V.sdiv(C2V));
> > +        if (C2V.isAllOnesValue() && C1V.isMinSignedValue())
> > +          return 0;        // MIN_INT / -1 -> overflow
> > +        return ConstantInt::get(C1->getType(), C1V.sdiv(C2V));
> 
> This isn't right, remove the first return.

Typo .. nice catch .. thanks.

> 
> >        case Instruction::Shl:
> > -        return ConstantInt::get(C1->getType(), C1Val << C2Val);
> > +        if (uint32_t shiftAmt = C2V.getZExtValue())
> > +          if (shiftAmt <= C1V.getBitWidth())
> 
> This should be <, not <=, likewise for other shifts.

Right.

> -Chris
> 
>