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

[Chris Lattner]

>> 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?

>>>    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.

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?

>> 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.

>>>
>>> @@ -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?

-Chris