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

[Reid Spencer]

On Tue, 2007-02-27 at 14:17 -0800, Chris Lattner wrote:
> >> 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.
> 
> Thanks, but why the default argument?

For compatibility with existing code that didn't have that parameter.
Call me lazy but I didn't feel like messing with all the test cases that
use this.

> 
> >>>    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.
> 
> The the interface takes a pointer to uint32_t or uint64_t, it doesn't  
> matter what the internal size is, it has to work with that array.

I have since made a bunch of things in the APInt interface private,
including these enum values.

> 
> Additionally, why are you checking this here?  What does  
> APINT_BITS_PER_WORD have to do with whether or not this operation can  
> be constant folded?

It doesn't. Don't know what I was thinking when I wrote that.

> 
> >> 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?
> 
> I'm just saying that instead of:   APInt::doFoo(x, bool), we should  
> have APInt::doFooSigned(x) and APInt::doFooUnsigned(x).  This is an  
> interface change, not a functionality change.

Did it several hours ago.

> 
> >>>
> >>> @@ -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).
> 
> Okay, can you add the other one?

You have two options:

1. Overload global operator== (bad practice in my books)
2. Deal with calls like: APIntOps::operator==(i, idxVal) which is uglier
than
   just reversing the operands.

What's the big deal about reversing the order of the operands?


> 
> -Chris
> 
>