[PATCH 2/2] IEEE-754R 2008 nextUp/nextDown implementation.

[Michael Gottesman]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130529/94130ad7/attachment.html>