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

mundaym · 2018-09-05T20:24:22Z

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.

The text was updated successfully, but these errors were encountered:

cherrymui · 2018-09-06T00:26:38Z

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

milanknezevic · 2018-09-07T16:33:45Z

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

go/src/cmd/compile/internal/ssa/check.go

Line 515 in 9facf35

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

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

randall77 · 2018-09-07T17:06:29Z

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

milanknezevic · 2018-09-10T17:45:57Z

@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 · 2018-09-12T11:25:42Z

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

mundaym · 2018-09-12T11:31:47Z

@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 · 2018-09-12T11:58:59Z

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 · 2018-09-12T12:12:38Z

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 · 2018-09-12T15:56:32Z

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 · 2018-09-12T16:55:14Z

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 · 2018-09-12T16:58:00Z

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 · 2018-09-12T17:59:01Z

@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 · 2018-09-12T18:02:26Z

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

gopherbot · 2018-09-13T12:34:42Z

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

milanknezevic · 2018-09-13T13:55:23Z

@mundaym Yes, it fixes the issue on mipsle.

randall77 · 2018-09-13T16:37:19Z

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 · 2018-09-13T16:37:31Z

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 · 2018-09-13T16:49:52Z

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

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>

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 added this to the Go1.12 milestone Sep 5, 2018

mundaym added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 5, 2018

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 added Unfortunate and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 13, 2018

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

bcmills mentioned this issue Sep 14, 2018

cmd/compile: 'panic during opt while compiling testfloat' on mipsle #27685

Closed

mundaym closed this as completed Sep 18, 2018

golang locked and limited conversation to collaborators Sep 18, 2019

gopherbot added the FrozenDueToAge label Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mundaym commented Sep 5, 2018

cherrymui commented Sep 6, 2018

milanknezevic commented Sep 7, 2018 •

edited

randall77 commented Sep 7, 2018

milanknezevic commented Sep 10, 2018

gopherbot commented Sep 12, 2018

mundaym commented Sep 12, 2018 •

edited

mundaym commented Sep 12, 2018 •

edited

mundaym commented Sep 12, 2018 •

edited

randall77 commented Sep 12, 2018

randall77 commented Sep 12, 2018

randall77 commented Sep 12, 2018

TocarIP commented Sep 12, 2018

randall77 commented Sep 12, 2018

gopherbot commented Sep 13, 2018

milanknezevic commented Sep 13, 2018

randall77 commented Sep 13, 2018

randall77 commented Sep 13, 2018

TocarIP commented Sep 13, 2018

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

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

Comments

mundaym commented Sep 5, 2018

cherrymui commented Sep 6, 2018

milanknezevic commented Sep 7, 2018 • edited

randall77 commented Sep 7, 2018

milanknezevic commented Sep 10, 2018

gopherbot commented Sep 12, 2018

mundaym commented Sep 12, 2018 • edited

mundaym commented Sep 12, 2018 • edited

mundaym commented Sep 12, 2018 • edited

randall77 commented Sep 12, 2018

randall77 commented Sep 12, 2018

randall77 commented Sep 12, 2018

TocarIP commented Sep 12, 2018

randall77 commented Sep 12, 2018

gopherbot commented Sep 13, 2018

milanknezevic commented Sep 13, 2018

randall77 commented Sep 13, 2018

randall77 commented Sep 13, 2018

TocarIP commented Sep 13, 2018

milanknezevic commented Sep 7, 2018 •

edited

mundaym commented Sep 12, 2018 •

edited

mundaym commented Sep 12, 2018 •

edited

mundaym commented Sep 12, 2018 •

edited