>> 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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
