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