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: math.Trunc produces incorrect result on riscv64 in Go 1.23rc1 #68322

Closed
mathetake opened this issue Jul 5, 2024 · 6 comments
Closed
Labels
arch-riscv Issues solely affecting the riscv64 architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mathetake
Copy link
Contributor

Go version

go version go1.23rc1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/mathetake/Library/Caches/go-build'
GOENV='/Users/mathetake/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/mathetake/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/mathetake/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/mathetake/sdk/go1.23rc1'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/mathetake/sdk/go1.23rc1/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23rc1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/mathetake/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/mathetake/wazero/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/zv/cj1015lx5bndpw3n_qsx9mh40000gn/T/go-build971855478=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I created the following reproducer

func TestRISCVTruncIncorrect(t *testing.T) {
	if math.Trunc(18446744073709549568.0) != 18446744073709549568.0 {
		t.Fatalf("math.Trunc(18446744073709549568.0) = %f, want 18446744073709549568.0", math.Trunc(18446744073709549568.0))
	}
}

What did you see happen?

I cross compiled the test with GOOS=linux GOARCH=riscv64, and run it in the qemu environment. Then the test passed with Go 1.22 but was failing with Go 1.23rc1.

What did you expect to see?

The test passes.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 5, 2024
@randall77
Copy link
Contributor

@golang/riscv64

@randall77 randall77 added the arch-riscv Issues solely affecting the riscv64 architecture. label Jul 5, 2024
@Jorropo
Copy link
Member

Jorropo commented Jul 6, 2024

Very probably ad377e9
cc @mengzhuo

@randall77
Copy link
Contributor

Yes, that riscv64 assembly looks bad. It converts float64->int64->float64, which doesn't work if the input float is bigger than 2^63-1. (For your example it is 2^64-2^11, I think, but generally it is a problem for very large floats.) We could probably compare |input| to a large value and if it is bigger than 2^53 (or whatever the last fractional float is), return the input. (And maybe we could do that instead of the Inf comparison?)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/596516 mentions this issue: math: remove riscv64 assembly implementations of rounding

@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 8, 2024
@cagedmantis cagedmantis added this to the Go1.23 milestone Jul 8, 2024
@Jorropo Jorropo assigned Jorropo and unassigned Jorropo Jul 10, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/600075 mentions this issue: math: add round assembly implementations on riscv64

gopherbot pushed a commit that referenced this issue Sep 11, 2024
This CL reapplies CL 504737 and adds integer precision
limitation check, since CL 504737 only checks whether
floating point number is +-Inf or NaN.

This CL is also ~7% faster than CL 504737.

Updates #68322

goos: linux
goarch: riscv64
pkg: math
            │ math.old.bench │           math.new.bench            │
            │     sec/op     │   sec/op     vs base                │
Ceil             54.09n ± 0%   18.72n ± 0%  -65.39% (p=0.000 n=10)
Floor            40.72n ± 0%   18.72n ± 0%  -54.03% (p=0.000 n=10)
Round            20.73n ± 0%   20.73n ± 0%        ~ (p=1.000 n=10)
RoundToEven      24.07n ± 0%   24.07n ± 0%        ~ (p=1.000 n=10)
Trunc            38.72n ± 0%   18.72n ± 0%  -51.65% (p=0.000 n=10)
geomean          33.56n        20.09n       -40.13%

Change-Id: I06cfe2cb9e2535cd705d40b6650a7e71fedd906c
Reviewed-on: https://go-review.googlesource.com/c/go/+/600075
Reviewed-by: Keith Randall <[email protected]>
Reviewed-by: Joel Sing <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv Issues solely affecting the riscv64 architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants