Fix NaN payload unittest in errorfunction

[smolt]

This is a mod that allows std.internal.math.errorfunction unittest to pass on several other platforms.
For some platforms, math on a NaN may change the sign. Replaced with test that ignores signbit for NaN.

Is this the appropriate? We are discussing it here: ldc-developers#25.

[rainers]

Is this the appropriate? We are discussing it here: ldc-developers#25.

I guess discussion should happen here for a wider audience.

Please note that dmd has the same issue when working with doubles instead of reals (on targets that use simd instructions for floating point):

void main()
{
    double b = double.nan;
    assert(*cast(long*)&b == *cast(long*)&b); // remove this assert to also fail for -O0
    double c = (-b) * b;
    double d = -(b * b);
    assert(*cast(long*)&c == *cast(long*)&d);
}

This fails with optimizations.

The problem is that the optimizer uses d ^ 0x8000_0000_0000_0000 as a replacement for -d disregarding nans which convert to ind instead of staying nan. The LLVM backend does the same.

[ibuclaw]

This seems reasonable.

[dnadlinger]

@rainers: How do you define ind as opposed to nan?

[rainers]

@rainers: How do you define ind as opposed to nan?

As much as I understand it, an ind is the result of an invalid computation with valid inputs like inf/inf. A 'nan' is the result of calculating with invalid inputs like nan or ind. This seems like a good description: http://www.codeproject.com/Articles/824516/Concept-of-NaN-IND-INF-and-DEN

[joakim-noah]

Only the second assert fails for me when natively compiled and run on Android/ARM, but this PR fixes it.

[rainers]

I think a comment would be nice why this is necessary at all, as it is not obvious. For example "some compilers implement '-x' by a bit operation on the sign bit, but this is inaccurate for NaNs. So better skip the sign bit when comparing the result of passing a NaN through some calculus."

[rainers]

@smolt could you please update this according to the comment https://github.com/D-Programming-Language/phobos/pull/3976/files#r52418960?

This is also a chance for others to object before I merge it...

[smolt]

@rainers ok, I'll add a comment. It could be a more general statement because C99 says this about C math functions:

• "F.9 (13) If a function with one or more NaN arguments returns a NaN result, the result should be the same as one of the NaN arguments (after possible type conversion), except perhaps for the sign."

and from wiki article on IEEE Floating Point:

• "The sign of a NaN has no meaning, but it may be predictable in some circumstances."
  implying unpredictable in other circumstances.

I think isNaNWithPayload is wrong in case x is a signaling NaN.

An alternative is to not add the new math function and just change the unittest to check for nan and then extract and compare payload.

[rainers]

@smolt Interesting. I wasn't aware that this isuue is even covered by the C standard.

I think isNaNWithPayload is wrong in case x is a signaling NaN.

Yeah, I guess it should just do what the name says:

    return isNaN(x) && getNaNPayload(x) == payload;

Making it a local function in the test should be fine, too.

[smolt]

I think I will do that, make it local to the test with your suggestion. I am away, you could just create new PR and close this one. I may not get to it until Saturday.

[smolt]

return isNaN(x) && getNaNPayload(x) == payload;

Doesn't work writing isNaNWithPayload like this. I think there is a bug in one of those two functions for 64-bit real. Investigating...

[smolt]

getNaNPayload() is off-by-one for 64-bit reals, shift should be 11-bits. The unittest that would have picked it up is only enabled for debug(UnitTest). Anybody know why?

I'll fix on this PR unless some thinks it should be a separate PR.

[smolt]

Ok, reworked along with fix for getNaNPayload. 64-bit real case tested with LDC on ARM. This is ready.

As a separate issue, compile std.math unittest with -O -debug=UnitTest and there appears to be some sort of dmd optimization problem. Is if familiar? It could explain why the unittest is disabled in a debug block. I'll create a bug report if needed.

[rainers]

compile std.math unittest with -O -debug=UnitTest and there appears to be some sort of dmd optimization problem

dmd takes the liberty to omit the reduction to double before calling getNaNPayload. I think that's ok for the optimizer to do. Making nan5 and nan6 static variables avoids this optimization and makes the (no-longer-pure) unittest pass.

There's also strange indentation at the end of getNaNPayload, it would be nice if you could fix that along the way.

I'll create a bug report if needed.

I think a bugzilla issue should be created for the fix to getNaNPayload.

[rainers]

I think a bugzilla issue should be created for the fix to getNaNPayload.

Just realized that the fixed branch never affects dmd because it always has 80-bit reals.

So I'll go ahead and merge this as is, other stuff can go into a separate PR.

[rainers]

Auto-merge toggled on

[smolt]

I think a bugzilla issue should be created for the fix to getNaNPayload.

It's on my TODO list.

[smolt]

did this stall?

[rainers]

It seems the build server forgot the merge state.

[rainers]

Auto-merge toggled on