[PATCH 2/2] IEEE-754R 2008 nextUp/nextDown implementation.
Michael Gottesman
mgottesman at apple.com
Thu May 30 11:09:15 PDT 2013
Committed in r182945.
On May 30, 2013, at 3:50 AM, Stephen Canon <scanon at apple.com> wrote:
> LGTM.
>
> On May 29, 2013, at 7:05 PM, Michael Gottesman <mgottesman at apple.com> wrote:
>
>> Hows this look:
>> <0001-APFloat-Implement-IEEE-754R-2008-nextUp-nextDown-fun.patch>
>>
>>
>> On May 29, 2013, at 1:52 PM, Michael Gottesman <mgottesman at apple.com> wrote:
>>
>>>
>>> On May 29, 2013, at 8:43 AM, Stephen Canon <scanon at apple.com> wrote:
>>>
>>>> On May 28, 2013, at 7:59 PM, Michael Gottesman <mgottesman at apple.com> wrote:
>>>>
>>>>> The attached patch implements IEEE-754R 2008 nextUp/nextDown via the new method APFloat::next.
>>>>
>>>> Hi Michael —
>>>>
>>>> First, spelling: “binade”, not “binaid”. This occurs at several points in the patch.
>>>
>>> Ok.
>>>
>>>>
>>>> Now, on to more specific comments:
>>>>
>>>> + /// Returns true if this is the smallest number by magnitude in the current
>>>> + /// semantics.
>>>> + bool isSmallest() const;
>>>> + /// Returns true if this is the largest number by magnitude in the current
>>>> + /// semantics.
>>>> + bool isLargest() const;
>>>>
>>>> I’m not convinced that these should be public. They are useful utility functions for implementing APFloat operations, but probably aren’t likely to be used otherwise.
>>>
>>> I made them public since it seemed that they would be in the same class as isDenormal (which is public). I am fine making them private.
>>>
>>>> Also, the comments are somewhat vague; is the intention that isSmallest return true for both either of ±MIN_DENORM, and false for all other values, or that true is returned only for +MIN_DENORM? If the latter, I would say “if this is the smallest strictly positive number in the current semantics”; if the former, I would clarify by adding “(of either sign)” or similar.
>>>
>>> I used the word magnitude specifically to signify that I was ignoring the sign. Additionally there is the issue of my not being clear that the number must be non-zero as well. (Steve and I spoke about this off list and agreed upon:
>>>
>>> ``Returns true if and only if the number has the smallest possible non-zero magnitude in the current semantics''
>>>
>>> and for isLargest:
>>>
>>> ``Returns true if and only if the number has the largest possible finite magnitude in the current semantics.'')
>>>
>>>>
>>>> +APFloat::isSmallest() const {
>>>> + // The smallest number by magnitude in our format will be the smallest
>>>> + // denormal, i.e. the floating point normal with exponent being minimum
>>>> + // exponent and significand bitwise equal to 1 (i.e. with MSB equal to 0).
>>>> + return isNormal() && exponent == semantics->minExponent &&
>>>> + significandMSB() == 0;
>>>> +}
>>>>
>>>> I stared at this for 10 minutes trying to makes sense of the comment in relation to the code. Apparently isNormal( ) is true for “denormal" numbers in APFloat(!?). I would suggest that “normal” is grossly incorrect terminology for the class actually being described (“[non-zero] finite numbers”), but that’s way outside the scope of this patch, so let’s ignore it for now. Having finally made sense of this, it appears to be correct.
>>>>
>>>> +void APFloat::makeLargest(bool Negative) {
>>>> // We want (in interchange format):
>>>> // sign = {Negative}
>>>> // exponent = 1..10
>>>> // significand = 1..1
>>>> -
>>>> - Val.exponent = Sem.maxExponent; // unbiased
>>>> + sign = Negative;
>>>> + exponent = semantics->maxExponent;
>>>>
>>>> // 1-initialize all bits....
>>>> - Val.zeroSignificand();
>>>> - integerPart *significand = Val.significandParts();
>>>> - unsigned N = partCountForBits(Sem.precision);
>>>> + zeroSignificand();
>>>>
>>>> You’re explicitly setting all bits; presumably zeroing them first is superfluous.
>>>
>>> This was just refactoring already written code. I will fix this
>>>
>>>>
>>>> + integerPart *significand = significandParts();
>>>> + unsigned N = partCountForBits(semantics->precision);
>>>> for (unsigned i = 0; i != N; ++i)
>>>> significand[i] = ~((integerPart) 0);
>>>>
>>>> Earlier in the patch you avoid the C-style cast and use integerPart(0). Not sure what LLVM style says, but you should be consistent.
>>>
>>> This was just refactoring already written code. I will fix this
>>>
>>>>
>>>> // ...and then clear the top bits for internal consistency.
>>>> - if (Sem.precision % integerPartWidth != 0)
>>>> + if (semantics->precision % integerPartWidth != 0)
>>>> significand[N-1] &=
>>>> - (((integerPart) 1) << (Sem.precision % integerPartWidth)) - 1;
>>>> + (((integerPart) 1) << (semantics->precision % integerPartWidth)) - 1;
>>>> +}
>>>>
>>>> Ditto. You could also just store the correct value of the high word of the integer part, rather than first setting it to all-ones in the loop and then masking it.
>>>
>>> This was just refactoring already written code. I will fix this.
>>>
>>>>
>>>> +void APFloat::makeSmallest(bool Negative) {
>>>> + // We want (in interchange format):
>>>> + // sign = {Negative}
>>>> + // exponent = 0..0
>>>> + // significand = 0..01
>>>> + sign = Negative;
>>>> + exponent = semantics->minExponent; // unbiased
>>>> + zeroSignificand();
>>>> + significandParts()[0] = 1;
>>>> +}
>>>>
>>>> The last two lines are cleaner as just APInt::tcSet(significandParts(), 1, partCount()), at least to my mind. Opinions may differ.
>>>
>>> This was just refactoring already written code. I will fix this.
>>>
>>>>
>>>> +bool APFloat::isSignaling() const {
>>>> + if (!isNaN())
>>>> + return false;
>>>> +
>>>> + // IEEE-754R 2008 6.2.1: A signaling NaN bit string should be encoded with the
>>>> + // first bit of the trailing significand being 0.
>>>> + return !APInt::tcExtractBit(significandParts(), semantics->precision - 2);
>>>> +}
>>>>
>>>> The signaling bit is a “should”, not a “shall”; historically some architectures *have* used other bits. Do we care? I don’t know. Probably not.
>>>
>>> I don't think we do, but we should at least add it to the header documentation that we are making this decision.
>>>
>>>>
>>>> + case fcNaN:
>>>> + // nextUp(sNaN) = sNaN. Set Invalid flag.
>>>> + //
>>>> + // According to IEEE-754R 2008, nextUp only signals Invalid Operation on
>>>> + // sNaN.
>>>> + if (isSignaling())
>>>> + result = opInvalidOp;
>>>> + // nextUp(qNaN) = qNaN
>>>> + break;
>>>>
>>>> Per IEEE-754, the result of nextUp(sNaN) is a qNaN, not the input sNaN. However, this is all a bit subtle as that holds when evaluation is being done at runtime and invalid can be signaled. Still, qNaN is probably the right result.
>>>
>>> I will fix this.
>>>
>>>>
>>>> + // nextUp(-getSmallest()) = -0
>>>> + if (isSmallest() && isNegative()) {
>>>> + APInt::tcSet(significandParts(), 0, partCount());
>>>> + exponent = 0;
>>>> + category = fcZero;
>>>> + break;
>>>> + }
>>>> +
>>>> + // nextUp(getLargest()) == INFINITY
>>>> + if (isLargest() && !isNegative()) {
>>>> + APInt::tcSet(significandParts(), 0, partCount());
>>>> + category = fcInfinity;
>>>> + exponent = semantics->maxExponent + 1;
>>>> + break;
>>>> + }
>>>>
>>>> Maybe add makeZero and makeInfinity methods? They should be generally at least as useful as makeSmallest / makeLargest.
>>>
>>> I was thinking of doing this but since it is not necessary for this specific patch I decided to abstain implementing them for now. I will prepare a separate patch once this is in to do that.
>>>
>>>>
>>>> + // We only cross a binaid boundary that requires adjusting the exponent
>>>> + // if:
>>>> + // 1. exponent != semantics->minExponent. This implies we are not in the
>>>> + // smallest binaid or are dealing with denormals.
>>>> + // 2. Our significand excluding the integral bit is all zeros.
>>>> + bool WillCrossBinaidBoundary =
>>>> + exponent != semantics->minExponent && isSignificandAllZeros();
>>>>
>>>> Is this test redundant? What would be the meaning of a number with all-zero significand and an exponent of minExponent?
>>>
>>> No it is not redundent. Remember isSignificandAllZeros is ignoring the implicit bit. Thus without the minExponent check, 0x1p-126 would return true. We want to just decrement said case since we represent explicitly the integral bit and represent denormals as having min exponent.
>>>
>>>>
>>>> + assert(exponent != semantics->maxExponent &&
>>>> + "We can not increment an exponent beyond the maxExponent allowed"
>>>> + " by the given floating point semantics.”);
>>>>
>>>> Is this assert needed? Wouldn’t this case have been already handled by the path for // nextUp(getLargest()) == INFINITY? nextUp/nextDown should be well-defined for all inputs.
>>>
>>> I put this in in case someone (doubtfully, but still) modifies the code and violates said invariant (that the case was handled previously). I can remove it if you want.
>>>
>>> Preparing updated patch.
>>>
>>> Michael
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130530/1dc9ffd8/attachment.html>
More information about the llvm-commits
mailing list