Skip to content

cmd/compile: Go round-trip fails float32->float64->float32 but C works with gcc 7.4.1 and 8.3.1 (-O0, ..., -O3, -Og, -Os) #36399

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

Closed
x448 opened this issue Jan 6, 2020 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@x448
Copy link

x448 commented Jan 6, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.5 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

linux/amd64

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/x448/.cache/go-build"
GOENV="/home/x448/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/x448/gocode1.13"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/x448/go1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/x448/go1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/x448/gocode1.13/mysrc/cbor-go/float16/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build276233946=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  • Cast float64 to float32 with sNaN value (turns into qNaN).
  • Cast float32 to float64 with sNaN value (keeps sNaN).

More at fxamacker/cbor#93

Bug reproducer:
https://play.golang.org/p/kzLI9A07wRv

package main

import (
	"fmt"
	"math"
)

func main() {
	// init float32 with sNaN
	const u32 = uint32(0x7f800001) // IEEE 754 binary32 representation of sNaN
	f32 := math.Float32frombits(u32)

	// cast float32 to float64
	f64 := float64(f32)
	u64 := math.Float64bits(f64)

	// test passes
	sbit64 := uint64(0x8000000000000) // the signalling/quiet bit for IEEE 754 binary64
	if (u64 & sbit64) != 0 {
		fmt.Printf("Go/Golang lost bit when casting float32 to float64, forced sNaN to qNaN\n")
	}

	f32bis := float32(f64)
	u32bis := math.Float32bits(f32bis)
	diff := u32bis ^ u32

	// test fails
	if diff != 0 {
		fmt.Printf("Go/Golang lost bit when casting float64 to float32, forced sNaN to qNaN\n")
		fmt.Printf("u32=0x%08x, u32bis=0x%08x, diff=0x%08x\n", u32, u32bis, diff)
	}

}

What did you expect to see?

Consistent behavior when casting from float64 to float32 and vice versa regarding the NaN signalling bit (quiet bit).

What did you see instead?

Inconsistent behavior when casting float with NaN signalling bit.

Go/Golang lost bit when casting float64 to float32, forced sNaN to qNaN
u32=0x7f800001, u32bis=0x7fc00001, diff=0x00400000

@randall77
Copy link
Contributor

This is the same thing that happens in C:

#include <stdio.h>
#include <stdint.h>
float f(float x) {
  return (float)(double)x;
}
int main(int argc, char *argv[]) {
  uint32_t x = 0x7f800001;
  float f32 = *(float*)(&x);
  float g32 = f(f32);
  uint32_t y = *(uint32_t*)(&g32);
  printf("%x %x %x\n", x, y, x^y);
}

Prints (using gcc at -O0, -O1 and greater optimize the conversions away):

7f800001 7fc00001 400000

Both Go and C use this pair:

	cvtss2sd	%xmm0, %xmm0
	cvtsd2ss	%xmm0, %xmm0

So this is just the underlying behavior of the hardware.
I don't see any easy way to fix this, even if I thought it was a bug. I'm not sure it is a bug, though.

@x448
Copy link
Author

x448 commented Jan 6, 2020

@randall77 holy cow, you already made progress in #36400! Thanks!

I wrote sNaN preserving conversion func today for float16 x448/float16#4 because its Fromfloat32 function was written to produce identical results as AMD/Intel F16C instructions (not always desirable for CBOR encoding.)

The new function in cbor-go/float16 (FromNaN32ps) converts NaN while preserving the signal/quiet bit. It's simple enough for Go to inline for float32 to float16 and could be modified to work with other sizes.

Please let me know if you need anything but I get the impression you already tracked down the root cause!

@randall77
Copy link
Contributor

I think the fix for #36400 is easy, if a bit tedious as far as lines of code goes.

I don't see any easy way to fix this one. I'm not convinced this problem deserves replacing all instances of float64->float32 conversion with something like FromNaN32ps. It's something like 2 dozen instructions.

You want it to preserve the signal/quiet bit, but what about the rest of the mantissa bits? We can't preserve them all.

As far as I can tell, the CVTSS2SD and CVTSD2SS instructions both converting signaling nans to quiet nans. It says so in the Intel docs (Intel® 64 and IA-32 Architectures Software Developer’s Manual, vol. 1, E-8):

For float32->float64

The double precision output QNaN1 is created from the single precision input SNaN as follows: the sign bit is preserved, the 8-bit exponent FFH is replaced by the 11-bit exponent 7FFH, and the 24-bit significand is extended to a 53-bit significand by pending 29 bits equal to 0. The second most significant bit of the significand is changed from 0 to 1 to convert the signaling NaN into a quiet NaN.

For float64->float32

The single precision output QNaN1 is created from the double precision input SNaN as follows: the sign bit is preserved, the 11-bit exponent 7FFH is replaced by the 8-bit exponent FFH, and the 53-bit significand is truncated to a 24-bit significand by removing its 29 least significant bits. The second most significant bit of the significand is changed from 0 to 1 to convert the signaling NaN into a quiet NaN.

Your test code above has the state of the signal/quiet bit backwards. The bit is 1 for quiet NaNs and 0 for signaling NaNs, so the test at line 19 should be reversed (when fixed, it will print that the signaling bit is lost in the float32->float64 direction also).

I think it's perfectly reasonable to define conversions to drop the signaling bit (in both directions). It isn't really necessary, but it seems that's how it's currently implemented, on x86 at least.

@x448 x448 changed the title Casting float64 to float32 turns sNaN into qNaN but casting float32 to float64 is OK Casting float64 to float32 turns sNaN into qNaN Jan 6, 2020
@x448 x448 changed the title Casting float64 to float32 turns sNaN into qNaN Casting float64 to float32 turns sNaN into qNaN but casting float32 to float64 is OK Jan 6, 2020
@x448
Copy link
Author

x448 commented Jan 6, 2020

@randall77 I could've chosen better variable names and messages. However,

  • My test code was correct (although poorly worded past bedtime).
  • The output I get with your C program is different from your output with gcc 7.4.1 and 8.3.1.
  • C round-trip succeeds with float32->float64->float32, using gcc 7.4.1 and 8.3.1
  • Go round-trip fails with float32->float64->float32.
  • Go casting float32 to float64 didn't change quiet bit.
  • Go casting float64 to float32 changed quiet bit to 1.

With gcc (using -O0, -Og, -O1, -O2, -O3, -Os, or no options) your C program always outputs:

7f800001 7f800001 0

Versions of gcc I tried:

  • gcc (SUSE Linux) 7.4.1 20190905 [gcc-7-branch revision 275407]
  • gcc (GCC) 8.3.1 20190223 (Red Hat 8.3.1-2)

Here's an improved reproducer in Go that prints:

u32=0x7f800001, u32&qbit32=0x0, quiet bit is 0 (sNaN)
First test...
u32=0x7f800001, u64=0x7ff0000020000000, u64&qbit64=0x0
Go casting float32 to float64 didn't change quiet bit
Second test...
u32=0x7f800001, u32bis=0x7fc00001, diff=0x400000, u32bis&qbit32=0x400000
Go casting float64 to float32 changed quiet bit to 1
Go failed round-trip casting float32->float64->float32, value changed
Go failed consistency test: got qforced32to64 != qforced64to32
Go doesn't always preserve or lose sNaN when casting

EDITED: Fixed cut/paste bug in new reproducer. Set qforced64to32 = true if needed. Same mistake in randall77's modified example is my fault, not his!

package main

import (
	"fmt"
	"math"
)

func main() {
	const qbit32 = uint32(1 << (23-1)) // quiet bit for IEEE 754 binary32
	const qbit64 = uint64(1 << (52-1)) // quiet bit for IEEE 754 binary64
	qforced32to64 := false
	qforced64to32 := false

	// init float32 with sNaN
	const u32 = uint32(0x7f800001) // sNaN because quiet bit is 0
	f32 := math.Float32frombits(u32)

	// make sure we starting with quiet bit = 0 (sNaN)
	if (u32 & qbit32) != 0 {
		fmt.Printf("Oops, the starting value is a qNaN.  Exiting now.\n")
		return
	} else {
		fmt.Printf("u32=0x%x, u32&qbit32=0x%x, quiet bit is 0 (sNaN)\n", u32, u32&qbit32)
	}

	// cast float32 to float64
	f64 := float64(f32)
	u64 := math.Float64bits(f64)

	// test passes
	fmt.Println("First test...")
	fmt.Printf("u32=0x%x, u64=0x%x, u64&qbit64=0x%x\n", u32, u64, u64&qbit64)
	if (u64 & qbit64) != 0 {
		fmt.Printf("Go casting float32 to float64 changed quiet bit to 1\n")
		qforced32to64 = true
	} else {
		fmt.Printf("Go casting float32 to float64 didn't change quiet bit\n")
	}

	f32bis := float32(f64)
	u32bis := math.Float32bits(f32bis)
	diff := u32bis ^ u32

	// test fails
	fmt.Println("Second test...")
	fmt.Printf("u32=0x%x, u32bis=0x%x, diff=0x%x, u32bis&qbit32=0x%x\n", u32, u32bis, diff, u32bis&qbit32)
	if (u32bis & qbit32) != 0 {
		fmt.Printf("Go casting float64 to float32 changed quiet bit to 1\n")
		qforced64to32 = true
	}
	if diff != 0 {
		fmt.Printf("Go failed round-trip casting float32->float64->float32, value changed\n")
	}

	if qforced32to64 != qforced64to32 {
		fmt.Printf("Go failed consistency test: got qforced32to64 != qforced64to32\n")
		fmt.Printf("Go doesn't always preserve or lose sNaN when casting\n")
	}
}

@x448 x448 changed the title Casting float64 to float32 turns sNaN into qNaN but casting float32 to float64 is OK Go round-trip fails float32->float64->float32 but C works with gcc 7.4.1 and 8.3.1 (-O0, ..., -O3, -Og, -Os) Jan 6, 2020
@randall77
Copy link
Contributor

Lots to figure out here...

First, my C code. You're right, on linux with gcc the signal/quiet bit persists on float32->float64->float32.
I was working on Darwin, on which gcc is actually just an alias for clang. (Damn you, Apple!)
Clang does force NaNs to quiet on float32->float64->float32 (at -O0).

For float32->float64->float32, gcc does:

    1139:       f3 0f 11 45 fc          movss  %xmm0,-0x4(%rbp)
    113e:       f3 0f 10 45 fc          movss  -0x4(%rbp),%xmm0

Even at -O0, it looks like gcc optimized out the conversion to float64 and back.

For float32->float64->float32, clang does:

  40113e:       f3 0f 5a c0             cvtss2sd %xmm0,%xmm0
  401142:       f2 0f 5a c0             cvtsd2ss %xmm0,%xmm0

Just like Go does.

Here's a reproducer that works with gcc also (at -O0 and -O1):

#include <stdio.h>
#include <stdint.h>
double f1(float x) {
  return (double)x;
}
float f2(double x) {
  return (float)x;
}
int main(int argc, char *argv[]) {
  uint32_t x = 0x7f800001;
  float f32 = *(float*)(&x);
  float g32 = f2(f1(f32));
  uint32_t y = *(uint32_t*)(&g32);
  printf("%x %x %x\n", x, y, x^y);
}

The code generated by gcc has this in f1:

    1135:       f3 0f 5a c0             cvtss2sd %xmm0,%xmm0

and this in f2:

    113a:       f2 0f 5a c0             cvtsd2ss %xmm0,%xmm0

You're right, I was incorrect about the error in your test code. I got confused by what was happening with the constant propagation in the compiler. Let's leave that issue for #36400 .
Rewriting your code to avoid #36400, I use:

package main

import (
	"fmt"
	"math"
)

var snan32 uint32 = 0x7f800001

func main() {
	const qbit32 = uint32(1 << (23 - 1)) // quiet bit for IEEE 754 binary32
	const qbit64 = uint64(1 << (52 - 1)) // quiet bit for IEEE 754 binary64
	qforced32to64 := false
	qforced64to32 := false

	// init float32 with sNaN
	u32 := uint32(snan32) // sNaN because quiet bit is 0
	f32 := math.Float32frombits(u32)

	// make sure we starting with quiet bit = 0 (sNaN)
	if (u32 & qbit32) != 0 {
		fmt.Printf("Oops, the starting value is a qNaN.  Exiting now.\n")
		return
	} else {
		fmt.Printf("u32=0x%x, u32&qbit32=0x%x, quiet bit is 0 (sNaN)\n", u32, u32&qbit32)
	}

	// cast float32 to float64
	f64 := float64(f32)
	u64 := math.Float64bits(f64)

	// test passes
	fmt.Println("First test...")
	fmt.Printf("u32=0x%x, u64=0x%x, u64&qbit64=0x%x\n", u32, u64, u64&qbit64)
	if (u64 & qbit64) != 0 {
		fmt.Printf("Go casting float32 to float64 changed quiet bit to 1\n")
		qforced32to64 = true
	} else {
		fmt.Printf("Go casting float32 to float64 didn't change quiet bit\n")
	}

	f32bis := float32(f64)
	u32bis := math.Float32bits(f32bis)
	diff := u32bis ^ u32

	// test fails
	fmt.Println("Second test...")
	fmt.Printf("u32=0x%x, u32bis=0x%x, diff=0x%x, u32bis&qbit32=0x%x\n", u32, u32bis, diff, u32bis&qbit32)
	if (u32bis & qbit32) != 0 {
		fmt.Printf("Go casting float64 to float32 changed quiet bit to 1\n")
		qforced32to64 = true
	}
	if diff != 0 {
		fmt.Printf("Go failed round-trip casting float32->float64->float32, value changed\n")
	}

	if qforced32to64 != qforced64to32 {
		fmt.Printf("Go failed consistency test: got qforced32to64 != qforced64to32\n")
		fmt.Printf("Go doesn't always preserve or lose sNaN when casting\n")
	}
}

This code shows Go sets the quiet bit in both conversions (32->64 and 64->32).
So does unoptimized C.

@ianlancetaylor
Copy link
Contributor

I can get GCC to turn on the quiet bit, via cvtss2sd, with this code:

#include <stdio.h>
#include <stdint.h>
__attribute__((noinline))
double d(double x) {
  return x;
}
__attribute__((noinline))
float f(float x) {
  return (float)d((double)x);
}
int main(int argc, char *argv[]) {
  uint32_t x = 0x7f800001;
  float f32 = *(float*)(&x);
  float g32 = f(f32);
  uint32_t y = *(uint32_t*)(&g32);
  printf("%x %x %x\n", x, y, x^y);
}

@x448
Copy link
Author

x448 commented Jan 6, 2020

I see! Thanks for the followups.

Do you kind folks know if Go always forces quiet bit = 1 for non-const floats when casting them on other architectures like ARMv8 and POWER8?

BTW, I fixed a cut/paste error in the new reproducer by updating the comment. The 2nd qforced32to64 = true should've been qforced64to32 = true.

@randall77
Copy link
Contributor

I don't know for sure about other architectures, but I suspect they do as well.
I can add a test for this behavior, and we can see.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/213477 mentions this issue: cmd/compile: don't allow NaNs in floating-point constant ops

@toothrot toothrot added this to the Go1.15 milestone Jan 7, 2020
@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 7, 2020
@ALTree ALTree changed the title Go round-trip fails float32->float64->float32 but C works with gcc 7.4.1 and 8.3.1 (-O0, ..., -O3, -Og, -Os) cmd/compile: Go round-trip fails float32->float64->float32 but C works with gcc 7.4.1 and 8.3.1 (-O0, ..., -O3, -Og, -Os) Jan 10, 2020
gopherbot pushed a commit that referenced this issue Feb 25, 2020
We store 32-bit floating point constants in a 64-bit field, by
converting that 32-bit float to 64-bit float to store it, and convert
it back to use it.

That works for *almost* all floating-point constants. The exception is
signaling NaNs. The round trip described above means we can't represent
a 32-bit signaling NaN, because conversions strip the signaling bit.

To fix this issue, just forbid NaNs as floating-point constants in SSA
form. This shouldn't affect any real-world code, as people seldom
constant-propagate NaNs (except in test code).

Additionally, NaNs are somewhat underspecified (which of the many NaNs
do you get when dividing 0/0?), so when cross-compiling there's a
danger of using the compiler machine's NaN regime for some math, and
the target machine's NaN regime for other math. Better to use the
target machine's NaN regime always.

This has been a bug since 1.10, and there's an easy workaround
(declare a global varaible containing the signaling NaN pattern, and
use that as the argument to math.Float32frombits) so we'll fix it in
1.15.

Fixes #36400
Update #36399

Change-Id: Icf155e743281560eda2eed953d19a829552ccfda
Reviewed-on: https://go-review.googlesource.com/c/go/+/213477
Run-TryBot: Keith Randall <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Josh Bleecher Snyder <[email protected]>
@randall77
Copy link
Contributor

randall77 commented Mar 3, 2020

Do you kind folks know if Go always forces quiet bit = 1 for non-const floats when casting them on other architectures like ARMv8 and POWER8?

Tests show that all platforms force the quiet bit.
But there's a big caveat: some MIPS platforms have an inverted idea of the quiet bit. i.e., they think that if the bit is set, it is signaling and if the bit is clear, it is quiet. This is the opposite of all of our other platforms (and even some of the other MIPS platforms).

@randall77
Copy link
Contributor

I'm going to close this issue as working as intended.
Conversions force NaNs to quiet. All of our platforms agree on this point. I don't see any strong reason to change the behavior, and lots of strong reasons not to.

@x448
Copy link
Author

x448 commented Mar 3, 2020

Thanks for looking into this and sharing your findings. Sounds like you made the right call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants