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.

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

randall77 commented Aug 18, 2022

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

randall77 commented Aug 18, 2022

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

randall77 commented Aug 18, 2022

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

randall77 commented Aug 19, 2022

@jmacd
Copy link
Author

jmacd commented Aug 19, 2022

Thank you @randall77 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Development

No branches or pull requests

3 participants