[PATCH] [APFloat] x-NaN needs to flip sign of NaN

[Steve (Numerics) Canon]

Committed revision 210428 (with "C++-style" comment ["C++-style"?  really?  C has supported them for 15 years now!]).

Thanks for the discussion.
– Steve

On Jun 8, 2014, at 9:41 AM, Michael Gottesman <mgottesman at apple.com> wrote:

> 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.
> 
> 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).
> 
> 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):
> 
>> 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:
>> 
>> 	z = copy(x)
>> 	z = negate(x)
>> 	z = abs(x)
>> 	z = copySign(x,y)
>> 
>> 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.
> 
> 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.
> 
> Thanks Steve!
> Michael
> 
> On Jun 7, 2014, at 11:44 PM, Steve (Numerics) Canon <scanon at apple.com> wrote:
> 
>> On Jun 7, 2014, at 11:25 PM, Alp Toker <alp at nuanti.com> wrote:
>> 
>>> On 08/06/2014 09:18, Steve (Numerics) Canon wrote:
>>>> 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.
>>>> 
>>>> This patch fixes http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-June/037388.html; 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.
>>>> 
>>>> 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.
>>> 
>>> Hi Steve,
>>> 
>>> This is one of those (rare) cases where a unit test is appropriate. Could you take a look at unittests/ADT/APFloatTest.cpp?
>>> 
>>> Alp.
>> 
>> Ah!  Thanks for the tip.
>> 
>> Now with tests:
>> 
>> <APFloat-negate.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>