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

[Chris Lattner]

On Feb 27, 2007, at 3:35 PM, Reid Spencer wrote:
> 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.

The test cases are temporary, right?  When they go, the default will  
go?  If you want, I can file a bug so that we don't forget this.

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

Thanks!

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

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

thanks

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

What's the problem with #1?  It's already overloaded for many other  
things, as is global operator<< (for printing).

-Chris