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

[Stephen Canon]

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.

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

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

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

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

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

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

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

+      // 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?

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

– Steve


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