<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;"><div>Thanks for updating the tests Steve. Also your patch LGTM except you need to change the C style comment to a C++ comment. In general in LLVM we only use C style comments in C source files.</div><div><br></div>To give some background to the list. There is a bug somewhere in APFloat that breaks commutativity with NaN when adding (IIRC I documented it in the unittests where we are checking that behavior).<div><br></div><div>In order to work around this inconsistency (and the lack of time I had at the moment to look into it), I took advantage of the clause in IEEE-754R which states that the semantic meaning of the sign bit of NaN is not specified. So I changed APFloat’s arithmetic operations to always emit a positive NaN since I could define the sign bit of a NaN to be semantically meaningless. But this creates an interesting issue (to quote Steve):</div><div><br></div><div><div></div><blockquote type="cite"><div>Whereas C, C++, and IEEE-754 do not care about the sign of NaN *in most contexts* (in fact, IEEE-754 says it has no semantic meaning in general), there are four "signbit" operations defined in IEEE-754 which are defined to "only effect the signbit" and "treating floating-point numbers and NaNs alike". I.e. when used in one of these four operations, the sign of NaN *does* have semantic meaning:</div><div><br></div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>z = copy(x)</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>z = negate(x)</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>z = abs(x)</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>z = copySign(x,y)</div><div><br></div><div>Those are the IEEE-754 names for the operations; the C and C++ mappings for them are assignment without implicit conversion, unary minus, fabs( ) and copysign( ), respectively. The trouble is that we don't have a "negate" operation; instead –x becomes 0 – x, which means that we need to make subtraction of NaN flip the sign bit in order to get the right behavior for negation.</div></blockquote><div><div><br></div><div>Since subtraction is never commutative this will not expose the commutativity bug so the change gives us more correctness without exposing further bugs in APFloat.</div><div><br></div><div>Thanks Steve!</div><div>Michael</div><div><br><div><div>On Jun 7, 2014, at 11:44 PM, Steve (Numerics) Canon <<a href="mailto:scanon@apple.com">scanon@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">On Jun 7, 2014, at 11:25 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:<br><br><blockquote type="cite">On 08/06/2014 09:18, Steve (Numerics) Canon wrote:<br><blockquote type="cite">This is simple enough for review-after-commit, but I don't often work on llvm, so I thought it best to send for review.<br><br>This patch fixes <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-June/037388.html">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-June/037388.html</a>; it turned out to be caused by APFloat not negating the sign of the NaN when producing the result of (0 – NaN). I made it flip the sign for x – NaN where x is any number. This isn't strictly required, but is allowed by IEEE-754, and was the minimal source change to get the needed behavior for the 0 – NaN case.<br><br>I have no idea how to write a test case for this bug; APFloat seems to be, ahem, "sparsely tested" at present, so there isn't much stylistic precedent.<br></blockquote><br>Hi Steve,<br><br>This is one of those (rare) cases where a unit test is appropriate. Could you take a look at unittests/ADT/APFloatTest.cpp?<br><br>Alp.<br></blockquote><br>Ah! Thanks for the tip.<br><br>Now with tests:<br><br><span><APFloat-negate.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></div></div></body></html>