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: possibly incorrect fused-multiply-add bypassing floating-point to integer conversions #54533

Closed
jmacd opened this issue Aug 18, 2022 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@jmacd
Copy link

jmacd commented Aug 18, 2022

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

$ go version
go version go1.18.5 darwin/arm64

Does this issue reproduce with the latest release?

This is the latest 1.18.x release (can't use 1.19.0, waiting for 1.19.1 to release b/c #54302).

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

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/josh.macdonald/Library/Caches/go-build"
GOENV="/Users/josh.macdonald/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/josh.macdonald/src/lightstep/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/josh.macdonald/src/lightstep/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/josh.macdonald/src/lightstep/go/src/github.com/lightstep/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/hp/8_6w4qns7g79tww_56kst5nh0000gp/T/go-build3568237414=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Ran the following:

package main

import "fmt"

func main() {
	fmt.Println("Test shows", test(0.99))
}

func test(q float64) string {
	p := q * 100
	d := int(p)
	f := p - float64(d)

	if f < 0 {
		panic(fmt.Sprint("impossible", p, d, f))
	}

	return fmt.Sprint(f)
}

What did you expect to see?

This should not panic.

What did you see instead?

I expect this not to panic because https://go.dev/ref/spec#Conversions states that floating-point to integer conversion rounds toward zero. Thus, the expression should not be negative. It looks like a fused-multiply-add is happening here, when the rules for conversion would not allow it.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 18, 2022
@randall77
Copy link
Contributor

My guess is that q * 100 is 99 - epsilon, but epsilon is small enough that float64(q*100) == 99. Then d==99, but q*100-99 in exact math is less than 0.

@randall77
Copy link
Contributor

In other words, int(p) is really int(float64(p)), and although int always rounds towards 0, float64 might round away from 0.

@jmacd
Copy link
Author

jmacd commented Aug 18, 2022

I see. Thank you for explaining. I think I was expecting the assignment p := q * 100 to compute a rounded intermediate result, but I see this is a consequence of the compiler having rewritten the expression to f := q * 100 - float64(int(q * 100)).

Is there a way to rewrite my code so that the intermediate result of rounding (p := q * 100) is forcibly used that would avoid the fused-multiply add optimization? I'm not surprised by this optimization happening inside an expression, I guess it's surprising across statements with intermediate variables.

@randall77
Copy link
Contributor

Yes, you can force rounding with a float64 cast: p := float64(q*100).

Closing, as not a bug.

@jmacd
Copy link
Author

jmacd commented Aug 19, 2022

Thank you. I confess I cannot find the explanation for how p := q * 100 (where q is float64) acts differently than p := float64(q * 100). Would you mind pointing to the explanation?

@randall77
Copy link
Contributor

@jmacd
Copy link
Author

jmacd commented Aug 19, 2022

Thank you @randall77 ❤️

@golang golang locked and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

3 participants