code review 6344078: math, runtime: use a NaN that matches gcc's

math, runtime: use a NaN that matches gcc's
        our old choice is not working properly at least on VFPv2 in
ARM1136JF-S (it's not preserved across float64->float32 conversions).

        Fixes issue 3745.

[dave_cheney.net, 2012-07-04 02:17:35]

LGTM. Tested on arm5 and arm7. pkg/math benchmarks show no performance
regression, but these two curious improvements when using VFP3

BenchmarkTanh              2301         2168   -5.78%
BenchmarkY0                2004         1855   -7.44%

http://codereview.appspot.com/6344078/diff/8002/src/pkg/math/bits_other.go
File src/pkg/math/bits_other.go (right):

http://codereview.appspot.com/6344078/diff/8002/src/pkg/math/bits_other.go#ne...
src/pkg/math/bits_other.go:5: // +build !arm
I'm not sold on the bits_other.go name. There isn't much of a precedent in the
std library, but maybe bits_x86.go would be better (until we support mips :)

http://codereview.appspot.com/6344078/diff/8002/src/pkg/runtime/float.c
File src/pkg/runtime/float.c (right):

http://codereview.appspot.com/6344078/diff/8002/src/pkg/runtime/float.c#newcode9
src/pkg/runtime/float.c:9: // ARM1136JF-S, quiet NaNs are not preserved across
conversion
I did not know this was a define, very useful.

[minux1, 2012-07-04 03:03:13]

still need to test on ARMv6, I can only verify fmt and math test passed,
as kindle doesn't have enough storage and memory to host a gcc toolchain
to do full ./all.bash testing.

On Tue, Jul 3, 2012 at 10:17 PM, <dave@cheney.net> wrote:

> but these two curious improvements when using VFP3
>
> BenchmarkTanh              2301         2168   -5.78%
> BenchmarkY0                2004         1855   -7.44%
>
i think it's placement related, as I only changed a constant for ARM.

>
> http://codereview.appspot.com/**6344078/diff/8002/src/pkg/**
>
math/bits_other.go#newcode5<http://codereview.appspot.com/6344078/diff/8002/src/pkg/math/bits_other.go#newcode5>
> src/pkg/math/bits_other.go:5: // +build !arm
> I'm not sold on the bits_other.go name. There isn't much of a precedent
> in the std library, but maybe bits_x86.go would be better (until we
> support mips :)
>
i think gccgo also uses the math package, so we actually do support mips :)

[minux1, 2012-07-05 19:44:24]

added a test for NaN() in math pkg so that we can catch the problem
sooner in the future.

[dave_cheney.net, 2012-07-11 04:51:37]

On 2012/07/05 19:44:24, minux wrote:
> added a test for NaN() in math pkg so that we can catch the problem
> sooner in the future.

LGTM. Tested on a real Pi

raspberrypi(~/go/src) % go test fmt math
ok      fmt     0.882s
ok      math    0.138s

[minux1, 2012-07-25 21:34:50]

any more comments on this CL?

[rsc, 2012-07-29 22:17:47]

Please mention in the comments that we're talking about
conversion to float32. I had to read the bug comments to 
figure that out (I notice it's also in the CL description, but
it should be in the code).

I am not sure this is the right fix. It seems to me that a
signaling NaN would, well, cause a signal that the OS would
need to handle, maybe even propagating to the user process.
Also, the usual behavior of a signaling NaN is that after the
signal it turns into a quiet NaN, which suggests that if you 
convert from a float64 to a float32 and back and then to a float32
again you will at some point end up at a quiet NaN and then have
the same problem as in the original.

Can we instead try to understand why the Raspberry Pi does 
not preserve the NaN on the float64 -> float32 conversion.
Are we using the wrong instruction, or setting an incorrect bit
in the instruction?

It feels like we are working around a processor bug by making
the code uglier, and worse it does not seem like a complete 
workaround.

[minux1, 2012-07-30 00:01:04]

On Sun, Jul 29, 2012 at 3:17 PM, <rsc@golang.org> wrote:

> I am not sure this is the right fix. It seems to me that a
> signaling NaN would, well, cause a signal that the OS would
> need to handle, maybe even propagating to the user process.
> Also, the usual behavior of a signaling NaN is that after the
> signal it turns into a quiet NaN, which suggests that if you
> convert from a float64 to a float32 and back and then to a float32
> again you will at some point end up at a quiet NaN and then have
> the same problem as in the original.
>
I've tried converting back and forth between float64 and float32, and
it is always NaN.

>
> Can we instead try to understand why the Raspberry Pi does
> not preserve the NaN on the float64 -> float32 conversion.
> Are we using the wrong instruction, or setting an incorrect bit
> in the instruction?
>
honestly, i don't understand this behavior of VFPv2 on ARM1136JF-S.
maybe a hardware bug, maybe it's linux vfp support code's buggy.

i've googled, but still couldn't get anything helpful on this matter.

but, i do find out a way to verify if the NaN i choose is correct,
i fount out that gcc will by default inlining calls to nan() in math.h,
and the value generated for nan("1") is (*):
.L2:
        .word   1
        .word   2146959360
which is 0x7ff8000000000001, and it is a signaling NaN.

I then tested amd64 version of gcc, and the value for nan("1") is
the same. (note that the amd64 has a different interpretation of
signaling/quiet NaNs, and this NaN on amd64 is a quiet NaN);
so my guess is that gcc really doesn't differentiate between NaNs
on different architectures, and always use this version.
Thus, I think we could do the same thing, what do you think?
(I should have mentioned that our NaN for amd64/386 is in fact
a signaling NaN)

*: I've used the gcc-4.6 provided in the Android NDK, version string is:
GCC: (GNU) 4.6.x-google 20120106 (prerelease)

[rsc, 2012-07-30 00:48:29]

So you're suggesting to use the same nan bits on all platforms, and
just change what bits we use?
That sounds fine. I'm much more comfortable with that than with
architecture-specific bits.

Russ

[minux1, 2012-07-30 01:45:43]

PTAL.

[rsc, 2012-08-03 18:03:13]

LGTM

http://codereview.appspot.com/6344078/diff/18004/src/pkg/math/bits.go
File src/pkg/math/bits.go (right):

http://codereview.appspot.com/6344078/diff/18004/src/pkg/math/bits.go#newcode8
src/pkg/math/bits.go:8: uvnan    = 0x7ff8000000000001
please use capital letters like the surrounding constants. 7FF

[rsc, 2012-08-03 18:03:36]

Update CL description too: drop "on ARM".

[minux1, 2012-08-07 01:57:05]

Hello dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[minux1, 2012-08-07 01:57:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=1f62df249175 ***

math, runtime: use a NaN that matches gcc's
        our old choice is not working properly at least on VFPv2 in
ARM1136JF-S (it's not preserved across float64->float32 conversions).

        Fixes issue 3745.

R=dave, rsc
CC=golang-dev
http://codereview.appspot.com/6344078

[dave_cheney.net, 2012-08-07 01:59:40]

outstanding.