<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 29, 2013, at 8:43 AM, Stephen Canon <<a href="mailto:scanon@apple.com">scanon@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On May 28, 2013, at 7:59 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.com</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">The attached patch implements IEEE-754R 2008 nextUp/nextDown via the new method APFloat::next.</div></blockquote><br></div><div dir="auto">Hi Michael —</div><div dir="auto"><br></div><div dir="auto">First, spelling: “binade”, not “binaid”. This occurs at several points in the patch.</div></div></blockquote><div><br></div><div>Ok.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div dir="auto"><br></div><div dir="auto">Now, on to more specific comments:</div><div dir="auto"><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div dir="auto"><br></div><div dir="auto"><div dir="auto">+ /// Returns true if this is the smallest number by magnitude in the current</div></div><div dir="auto"><div dir="auto">+ /// semantics.</div></div><div dir="auto"><div dir="auto">+ bool isSmallest() const;</div></div><div dir="auto"><div dir="auto">+ /// Returns true if this is the largest number by magnitude in the current</div></div><div dir="auto"><div dir="auto">+ /// semantics.</div></div><div dir="auto"><div dir="auto">+ bool isLargest() const;</div></div></blockquote></div></div></blockquote><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div dir="auto"><div dir="auto"><br></div><div dir="auto">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.</div></div></div></blockquote><div><br></div><div>I made them public since it seemed that they would be in the same class as isDenormal (which <i>is</i> public). I am fine making them private.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div dir="auto"><div dir="auto">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.</div></div></div></blockquote><div><br></div><div>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:</div><div><br></div><div><div style="margin: 0px;">``Returns true if and only if the number has the smallest possible non-zero magnitude in the current semantics''</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">and for isLargest:</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">``Returns true if and only if the number has the largest possible finite magnitude in the current semantics.'')</div></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div dir="auto"><div dir="auto"><br></div></div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div dir="auto"><div dir="auto"><div dir="auto">+APFloat::isSmallest() const {</div><div dir="auto">+ // The smallest number by magnitude in our format will be the smallest</div><div dir="auto">+ // denormal, i.e. the floating point normal with exponent being minimum</div><div dir="auto">+ // exponent and significand bitwise equal to 1 (i.e. with MSB equal to 0).</div><div dir="auto">+ return isNormal() && exponent == semantics->minExponent &&</div><div dir="auto">+ significandMSB() == 0;</div><div dir="auto">+}</div><div dir="auto"><br></div></div></div></blockquote>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.<div><br></div><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div>+void APFloat::makeLargest(bool Negative) {</div><div> // We want (in interchange format):</div><div> // sign = {Negative}</div><div> // exponent = 1..10</div><div> // significand = 1..1</div><div>-</div><div>- Val.exponent = Sem.maxExponent; // unbiased</div><div>+ sign = Negative;</div><div>+ exponent = semantics->maxExponent;</div><div> </div><div> // 1-initialize all bits....</div><div>- Val.zeroSignificand();</div><div>- integerPart *significand = Val.significandParts();</div><div>- unsigned N = partCountForBits(Sem.precision);</div><div>+ zeroSignificand();</div><div><br></div></blockquote>You’re explicitly setting all bits; presumably zeroing them first is superfluous.<br></div></div></blockquote><div><br></div><div>This was just refactoring already written code. I will fix this</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><br></div><div>+ integerPart *significand = significandParts();</div><div>+ unsigned N = partCountForBits(semantics->precision);</div><div> for (unsigned i = 0; i != N; ++i)</div><div> significand[i] = ~((integerPart) 0);</div><div><br></div></blockquote>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.<br></div></div></blockquote><div><br></div><div>This was just refactoring already written code. I will fix this</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><div> </div><div> // ...and then clear the top bits for internal consistency.</div><div>- if (Sem.precision % integerPartWidth != 0)</div><div>+ if (semantics->precision % integerPartWidth != 0)</div><div> significand[N-1] &=</div><div>- (((integerPart) 1) << (Sem.precision % integerPartWidth)) - 1;</div><div>+ (((integerPart) 1) << (semantics->precision % integerPartWidth)) - 1;</div><div>+}</div></div><div><br></div></blockquote>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.</div></div></blockquote><div><br></div><div>This was just refactoring already written code. I will fix this.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div>+void APFloat::makeSmallest(bool Negative) {</div><div>+ // We want (in interchange format):</div><div>+ // sign = {Negative}</div><div>+ // exponent = 0..0</div><div>+ // significand = 0..01</div><div>+ sign = Negative;</div><div>+ exponent = semantics->minExponent; // unbiased</div><div>+ zeroSignificand();</div><div>+ significandParts()[0] = 1;</div><div>+}</div><div><br></div></blockquote>The last two lines are cleaner as just APInt::tcSet(significandParts(), 1, partCount()), at least to my mind. Opinions may differ.</div></blockquote><div><br></div><div>This was just refactoring already written code. I will fix this.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><div>+bool APFloat::isSignaling() const {</div><div>+ if (!isNaN())</div><div>+ return false;</div><div>+</div><div>+ // IEEE-754R 2008 6.2.1: A signaling NaN bit string should be encoded with the</div><div>+ // first bit of the trailing significand being 0.</div><div>+ return !APInt::tcExtractBit(significandParts(), semantics->precision - 2);</div><div>+}</div></div><div><br></div></blockquote>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.</div></div></blockquote><div><br></div><div>I don't think we do, but we should at least add it to the header documentation that we are making this decision.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><div>+ case fcNaN:</div><div>+ // nextUp(sNaN) = sNaN. Set Invalid flag.</div><div>+ //</div><div>+ // According to IEEE-754R 2008, nextUp only signals Invalid Operation on</div><div>+ // sNaN.</div><div>+ if (isSignaling())</div><div>+ result = opInvalidOp;</div><div>+ // nextUp(qNaN) = qNaN</div><div>+ break;</div></div><div><br></div></blockquote>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.</div></div></blockquote><div><br></div><div>I will fix this.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div>+ // nextUp(-getSmallest()) = -0</div><div>+ if (isSmallest() && isNegative()) {</div><div>+ APInt::tcSet(significandParts(), 0, partCount());</div><div>+ exponent = 0;</div><div>+ category = fcZero;</div><div>+ break;</div><div>+ }</div><div>+</div><div>+ // nextUp(getLargest()) == INFINITY</div><div>+ if (isLargest() && !isNegative()) {</div><div>+ APInt::tcSet(significandParts(), 0, partCount());</div><div>+ category = fcInfinity;</div><div>+ exponent = semantics->maxExponent + 1;</div><div>+ break;</div><div>+ }</div><div><br></div></blockquote>Maybe add makeZero and makeInfinity methods? They should be generally at least as useful as makeSmallest / makeLargest.</div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><div>+ // We only cross a binaid boundary that requires adjusting the exponent</div><div>+ // if:</div><div>+ // 1. exponent != semantics->minExponent. This implies we are not in the</div><div>+ // smallest binaid or are dealing with denormals.</div><div>+ // 2. Our significand excluding the integral bit is all zeros.</div><div>+ bool WillCrossBinaidBoundary =</div><div>+ exponent != semantics->minExponent && isSignificandAllZeros();</div></div><div><br></div></blockquote>Is this test redundant? What would be the meaning of a number with all-zero significand and an exponent of minExponent?</div></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote style="margin: 0px 0px 0px 40px; border: none; padding: 0px;"><div><div><br></div><div>+ assert(exponent != semantics->maxExponent &&</div><div>+ "We can not increment an exponent beyond the maxExponent allowed"</div><div>+ " by the given floating point semantics.”);</div></div><div><br></div></blockquote>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.</div></div></blockquote><div><br></div><div>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.</div></div><br><div>Preparing updated patch.</div><div><br></div><div>Michael</div><div><br></div></body></html>