Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: 387 quietens signalling NaNs during compilation #27516

Closed
mundaym opened this issue Sep 5, 2018 · 18 comments

Comments

Projects
None yet
6 participants
@mundaym
Copy link
Member

commented Sep 5, 2018

The compiler, when built using GOARCH=386 GO386=387, is generating the wrong code for this test. It is still quietening float32 signalling NaNs that are constant folded (see issue #27193). We are either still relying on floating point behavior somewhere in the compiler that is undefined or (less likely) there is a bug in the 387 backend.

@mundaym mundaym added this to the Go1.12 milestone Sep 5, 2018

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

The mipsle builder is also failing, with a different error though.
https://build.golang.org/log/453d116005d8a95c02b773e969674ddd9f74a6c6

@milanknezevic

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

The error on mipsle is caused by NaN value of 0x7FF7FFFFFFFFFFFF which causes this condition to be true.

if bits.TrailingZeros64(math.Float64bits(x)) < 52-23 {

Maybe !math.isNaN(x) check should be added to the condition.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@milanknezevic What function is being compiled when this happens? Where is it getting that NaN value from?

@milanknezevic

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@randall77

What function is being compiled when this happens?

This is the error message: ./map.go:515:31: internal compiler error: 'testfloat': panic during opt while compiling testfloat, but the same behavior on mipsle can be reproduced with this test case.

Where is it getting that NaN value from?

After constant folding (Cvt64Fto32F) of value returned by math.NaN(), AuxInt field contains 0x7FF7FFFFFFFFFFFF.
Just to mention, 0x7FF8000000000001 which is returned by math.NaN() is signaling NaN on mips, not quiet NaN as on other architectures. So when we do this constant folding on mips hardware we actually get 0x7FF7FFFFFFFFFFFF.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 12, 2018

Change https://golang.org/cl/134855 mentions this issue: cmd/compile: avoid more float32 <-> float64 conversions in compiler

@mundaym

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@milanknezevic Does CL 134855 help at all? It should stop ones being appended to NaN values after a float64 -> float32 conversion (to convert them back into float64 values for storage in an AuxInt).

@mundaym

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

The 387 failure seems to exist even when constant propagation is disabled. So I guess even just loading a float32 into the 387 unit quietens the NaN? Does anyone know why this happens with 387?

I applied the following patch to the generic rules (at CL 134855):

diff --git a/src/cmd/compile/internal/ssa/gen/generic.rules b/src/cmd/compile/internal/ssa/gen/generic.rules
index cdf425a7e0..ec25dc10ba 100644
--- a/src/cmd/compile/internal/ssa/gen/generic.rules
+++ b/src/cmd/compile/internal/ssa/gen/generic.rules
@@ -44,8 +44,8 @@
 (Trunc64to8  (Const64  [c])) -> (Const8   [int64(int8(c))])
 (Trunc64to16 (Const64  [c])) -> (Const16  [int64(int16(c))])
 (Trunc64to32 (Const64  [c])) -> (Const32  [int64(int32(c))])
-(Cvt64Fto32F (Const64F [c])) -> (Const32F [f2i(extend32Fto64F(float32(i2f(c))))])
-(Cvt32Fto64F (Const32F [c])) -> (Const64F [c]) // c is already a 64 bit float
+//(Cvt64Fto32F (Const64F [c])) -> (Const32F [f2i(extend32Fto64F(float32(i2f(c))))])
+//(Cvt32Fto64F (Const32F [c])) -> (Const64F [c]) // c is already a 64 bit float
 (Cvt32to32F  (Const32  [c])) -> (Const32F [f2i(extend32Fto64F(float32(int32(c))))])
 (Cvt32to64F  (Const32  [c])) -> (Const64F [f2i(float64(int32(c)))])
 (Cvt64to32F  (Const64  [c])) -> (Const32F [f2i(extend32Fto64F(float32(c)))])
@@ -571,10 +571,10 @@
        -> x
 
 // Pass constants through math.Float{32,64}bits and math.Float{32,64}frombits
-(Load <t1> p1 (Store {t2} p2 (Const64  [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 8 && is64BitFloat(t1) -> (Const64F [x])
-(Load <t1> p1 (Store {t2} p2 (Const32  [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 4 && is32BitFloat(t1) -> (Const32F [f2i(extend32Fto64F(math.Float32frombits(uint32(x))))])
-(Load <t1> p1 (Store {t2} p2 (Const64F [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 8 && is64BitInt(t1)   -> (Const64  [x])
-(Load <t1> p1 (Store {t2} p2 (Const32F [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 4 && is32BitInt(t1)   -> (Const32  [int64(int32(math.Float32bits(truncate64Fto32F(i2f(x)))))])
+//(Load <t1> p1 (Store {t2} p2 (Const64  [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 8 && is64BitFloat(t1) -> (Const64F [x])
+//(Load <t1> p1 (Store {t2} p2 (Const32  [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 4 && is32BitFloat(t1) -> (Const32F [f2i(extend32Fto64F(math.Float32frombits(uint32(x))))])
+//(Load <t1> p1 (Store {t2} p2 (Const64F [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 8 && is64BitInt(t1)   -> (Const64  [x])
+//(Load <t1> p1 (Store {t2} p2 (Const3https://play.golang.org/p/W8Su1EmcRjc2F [x]) _)) && isSamePtr(p1,p2) && sizeof(t2) == 4 && is32BitInt(t1)   -> (Const32  [int64(int32(math.Float32bits(truncate64Fto32F(i2f(x)))))])
 
 // Float Loads up to Zeros so they can be constant folded.
 (Load <t1> op:(OffPtr [o1] p1)

A smaller example is here: https://play.golang.org/p/W8Su1EmcRjc

This code compiles into this 387 assembler (with the patch above applied and the rules regenerated):

    00000 (8) TEXT "".main(SB)
    00001 (8) FUNCDATA $0, gclocals·69c1753bd5f81501d95132d08af04464(SB)
    00002 (8) FUNCDATA $1, gclocals·cab29d17fa8bdbfaac1234d7f8bbe386(SB)
    00003 (8) FUNCDATA $3, gclocals·df4642631589a950a30d9cb77310f3ae(SB)
b3
    00004 (+10) PCDATA $2, $0
    00005 (+10) PCDATA $0, $0
    00006 (+10) MOVL $2140003454, math.b-36(SP)
    # $GOROOT/src/math/unsafe.go
    00007 (+14) FMOVF math.b-36(SP), F0
    # /home/michael/Development/go/scratch/f32.go
    00008 (+10) FMOVD F0, F0
    00009 (10) FMOVFP F0, math.f-40(SP)
    # $GOROOT/src/math/unsafe.go
    00010 (10) MOVL math.f-40(SP), CX
    # /home/michael/Development/go/scratch/f32.go
    00011 (+11) CMPL CX, $2140003454
    00012 (11) FMOVDP F0, F0
    00013 (11) JNE 17
    00014 (?) PCDATA $2, $-2
    00015 (?) PCDATA $0, $-2
    00016 (?) RET
    # $GOROOT/src/math/unsafe.go
    00017 (10) PCDATA $2, $0
    00018 (10) PCDATA $0, $0
    00019 (10) MOVL CX, "".got-44(SP)
    # /home/michael/Development/go/scratch/f32.go
    ....
@mundaym

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

Looks like the flds instruction (FMOVF math.b-36(SP), F0 in Go) is extending the float32 to float80 to put it on the x87 stack and in the process quietens the NaN. Not sure what we want to do about this...

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

The x86 manual divides 387 operations into "arithmetic" and "non-arithmetic". FLD of an 80-bit value is non-arithmetic. FLD of a 64 or 32 bit value is arithmetic, however.
The reason it matters is that for arithmetic instructions:

Any arithmetic operation on a SNaN. | Return a QNaN to the destination operand (see Table 4-7).

It also sets the IE flag of the FPU status word. We could conceivably detect that and un-quiet the NaN in that case.

@TocarIP

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

This is a problem for both float32 and float64. My repro:

package main

import (
	"fmt"
	"unsafe"
)

var x32, y32 float32

//go:noinline
func copy32() {
	y32 = x32
}
func test32() {
	var want uint32 = 0x7f800001
	*(*uint32)(unsafe.Pointer(&x32)) = want
	copy32()
	got := *(*uint32)(unsafe.Pointer(&y32))
	if got != want {
		fmt.Printf("got=%x, want=%x\n", got, want)
	}
}

var x64, y64 float64

//go:noinline
func copy64() {
	y64 = x64
}
func test64() {
	var want uint64 = 0x7ff0000000000001
	*(*uint64)(unsafe.Pointer(&x64)) = want
	copy64()
	got := *(*uint64)(unsafe.Pointer(&y64))
	if got != want {
		fmt.Printf("got=%x, want=%x\n", got, want)
	}
}

func main() {
	test32()
	test64()
}
$ GOARCH=386 GO386=387 ~/go1.11/bin/go run tmp1.go
got=7fc00001, want=7f800001
got=7ff8000000000001, want=7ff0000000000001
@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Fixing this is going to be tough. Both loads and stores need to be fixed. I'm inclined to just declare that 387 doesn't do signaling NaNs correctly, and leave it at that. We can disable the tests for 387.

@TocarIP

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@randall77 wont this cause different code gen due to use of Float32frombits in compiler and result in different behavior of binaries built on 387 and all others?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@TocarIP : yes. Both running on 387 and compiling on 387 would be broken.
I don't think anyone is cross-compiling from 387.

@mundaym mundaym changed the title cmd/compile: linux-386-387 builder failing new TestFloat32StoreToLoadConstantFold test cmd/compile: 387 quietens signalling NaNs during compilation Sep 13, 2018

@mundaym mundaym modified the milestones: Go1.12, Unplanned Sep 13, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Sep 13, 2018

Change https://golang.org/cl/135195 mentions this issue: cmd/compile: skip float32 constant folding test on 387 builder

@milanknezevic

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

@mundaym Yes, it fixes the issue on mipsle.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Pretty easy to trigger this issue in C:

#include <stdio.h>

float x;
float y;

typedef unsigned long uint32;

int main(int argc, char* argv[]) {
  uint32 want = 0x7f800001;
  *(uint32*)(&x) = want;
  y = x;
  uint32 got = *(uint32*)(&y);
  if (want != got) {
    printf("%x %x\n", want, got);
  }
  return 0;
}

Compile with -m32 -march=i386.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Here's a very interesting read about this problem in Java.

java.lang.Double.longBitsToDouble says this:

Note that this method may not be able to return a double NaN with exactly same bit pattern as the long argument. IEEE 754 distinguishes between two kinds of NaNs, quiet NaNs and signaling NaNs. The differences between the two kinds of NaN are generally not visible in Java. Arithmetic operations on signaling NaNs turn them into quiet NaNs with a different, but often similar, bit pattern. However, on some processors merely copying a signaling NaN also performs that conversion. In particular, copying a signaling NaN to return it to the calling method may perform this conversion. So longBitsToDouble may not be able to return a double with a signaling NaN bit pattern. Consequently, for some long values, doubleToRawLongBits(longBitsToDouble(start)) may not equal start. Moreover, which particular bit patterns represent signaling NaNs is platform dependent; although all NaN bit patterns, quiet or signaling, must be in the NaN range identified above.

@TocarIP

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

It is even worse in C, due to ABI mandating x87 for returning values gcc bug

gopherbot pushed a commit that referenced this issue Sep 14, 2018

cmd/compile: skip float32 constant folding test on 387 builder
The 387 unit always quietens float64 and float32 signaling NaNs,
even when just loading and storing them. This makes it difficult
to propagate such values in the compiler. This is a hard problem
to fix and it is also very obscure.

Updates #27516.

Change-Id: I03d88e31f14c86fa682fcea4b6d1fba18968aee8
Reviewed-on: https://go-review.googlesource.com/135195
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit that referenced this issue Sep 17, 2018

cmd/compile: avoid more float32 <-> float64 conversions in compiler
Use the new custom truncate/extension code when storing or extracting
float32 values from AuxInts to avoid the value being changed by the
host platform's floating point conversion instructions (e.g. sNaN ->
qNaN).

Updates #27516.

Change-Id: Id39650f1431ef74af088c895cf4738ea5fa87974
Reviewed-on: https://go-review.googlesource.com/134855
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

@mundaym mundaym closed this Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.