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

Jump to bottom

cmd/compile: inconsistent float64 behaviour between arm64 and amd64 #36536

Closed
kortschak opened this issue Jan 13, 2020 · 7 comments
Closed

cmd/compile: inconsistent float64 behaviour between arm64 and amd64 #36536

kortschak opened this issue Jan 13, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@kortschak
Copy link
Contributor

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

$ go version
go version go1.13.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/user/bin"
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build254211515=/tmp/go-build -gno-record-gcc-switches"

What did you do?

~ $ cat inv.go 
package main

import "fmt"

func main() {
	nNeg := 10.0
	fpr := []float64{0, 10}
	invNeg := 1 / nNeg
	for i := range fpr {
		fpr[i] = 1 - fpr[i]*invNeg
	}
	fmt.Println(fpr)
}
~ $ cat div.go 
package main

import "fmt"

func main() {
	nNeg := 10.0
	fpr := []float64{0, 10}
	for i := range fpr {
		fpr[i] = 1 - fpr[i]/nNeg
	}
	fmt.Println(fpr)
}
~ $ GOARCH=amd64 go run inv.go 
[1 0]
~ $ GOARCH=amd64 go run div.go 
[1 0]
~ $ GOARCH=arm64 go run inv.go 
[1 -5.551115123125783e-17]
~ $ GOARCH=arm64 go run div.go 
[1 0]

inv.go: https://play.golang.org/p/yqox_9PRxI1
div.go: https://play.golang.org/p/8HhGeB-rUpn

Note that while this terminal session is all run on one machine using qemu-user for arm64, the same behaviour is seen on real arm64 hardware.

What did you expect to see?

Matching behaviour between arm64 and amd64.

What did you see instead?

Floating point difference for one approach.

This difference is only seen when the floats are in a slice, the equivalent in a single value behaves as expected.

~ $ cat inv_single.go 
package main

import "fmt"

func main() {
	nNeg := 10.0
	fpr := 10.0
	invNeg := 1 / nNeg
	fpr = 1 - fpr*invNeg
	fmt.Println(fpr)
}
~ $ GOARCH=amd64 go run inv_single.go 
0
~ $ GOARCH=arm64 go run inv_single.go 
0
@kortschak kortschak mentioned this issue Jan 13, 2020
Closed
25 tasks
@ianlancetaylor
Copy link
Contributor

CC @randall77 @griesemer

The spec says (https://golang.org/ref/spec#Arithmetic_operators):

An implementation may combine multiple floating-point operations into a single fused operation, possibly across statements, and produce a result that differs from the value obtained by executing and rounding the instructions individually. An explicit floating-point type conversion rounds to the precision of the target type, preventing fusion that would discard that rounding.

That is, Go has never promised to produce bit-identical results on different architectures. I suspect that is what is happening here, but leaving the issue open for investigation or discussion.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 13, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jan 13, 2020
@kortschak
Copy link
Contributor Author

kortschak commented Jan 13, 2020 •
edited

Yes, that's why I asked at go-nuts. The thing that tipped the balance is that I see a difference between in slice and single value versions of the code. Also note that the original code where the issue was identified expressed the mul and add on different lines, and so should not have been fused. Despite this, amd64 behaved the same way, and I am unable to get the same behaviour as arm64 by interposing explicit float64 conversions within the expression.

@kortschak
Copy link
Contributor Author

So it does look like it's due to that, just not in the direction I was thinking. On arm64 a fused multiply sub instruction is generated, but on amd64, a mul and sub set are emitted. Adding an explicit float64 conversion makes the arm64 behaviour match amd64.

@randall77
Copy link
Contributor

Yes, this is the arm64 backend using a fused multiply-subtract to compute the result. invNeg is not exactly 1/10, so 1 - (~1/10)*10 is not exactly zero. But (~1/10)*10 is within an epsilon of 1 (epsilon is around 10^-16 here), so the intermediate rounding gives a result of 0 on other platforms.

@kortschak
Copy link
Contributor Author

Thanks. So the surprising thing is that in the old code, where the expressions were over two lines

		fpr[i] *= invNeg
		fpr[i] = 1 - fpr[i]

the fused operation is still generated. I would have thought that the *= did a float64 conversion.

@randall77
Copy link
Contributor

You can only prohibit the fusing with an explicit float64 cast.

@kortschak
Copy link
Contributor Author

kortschak commented Jan 13, 2020 •
edited

Yes, just confirmed that, the second line would need to be fpr[i] = 1 - float64(fpr[i]). It seems the SSA analysis does an amazing job at finding diffuse objects (though interestingly not if there are separating lines)

@golang golang locked and limited conversation to collaborators Jan 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@kortschak @ianlancetaylor @randall77 @gopherbot