-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/image/vector TestMakeFlInXxx fails on s390x and ppc64le #21460
Comments
Thanks for the report. This is almost certainly due to the new fused multiply add (FMA) support on these architectures. We could:
Any thoughts @nigeltao? |
I started a golang-dev thread at https://groups.google.com/d/topic/golang-dev/Sti0bl2xUXQ/discussion |
Based on that discussion, I think the right fix for now is to explicitly write "float32(xy)" instead of just "xy" all throughout the Rasterizer.floatingLineTo method in raster_floating.go. I don't have access to s390x or ppc64le hardware, so somebody else will have to write (and verify) the patch. |
Oh, and when somebody does write the patch, it'd also be nice to get some benchmark numbers to see how much an effect FMA has. If it really is a massive speed-up, it might be worth considering loosening the design so that the output is merely close, not always identical. |
Change https://golang.org/cl/61024 mentions this issue: |
I sent CL 61024. It's a bit ugly but it appears to fix the tests. I also played with the benchmarks a bit. It looks like the floating point code is only really used for testing, is that correct? I modified the benchmarks to use the floating point implementation and the CL didn't have a big effect (about 1% max). The |
Ah, yes, the comment on floatingPointMathThreshold in vector.go says:
So we didn't have a vector_test.go benchmark that tickles the floating point code path. We had acc_test.go benchmarks that do, but they're narrower in scope, rather than end-to-end. I've just submitted https://go-review.googlesource.com/c/image/+/62130 to fix that. |
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
The test code now produces bit-identical results across platforms. Fixes golang/go#21460. Change-Id: I58b72dacf3c7930e2804b4e37bd3ccd9ef3dc2f0 Reviewed-on: https://go-review.googlesource.com/61024 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Nigel Tao <nigeltao@golang.org>
go version
go version go1.9beta2 linux/ppc64le
and
go version go1.9beta2 linux/s390x
go env for ppc64le:
GOARCH="ppc64le"
GOBIN=""
GOEXE=""
GOHOSTARCH="ppc64le"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/builddir/build/BUILDROOT/golang-github-golang-image-0-0.2.20170514.git426cfd8.fc27.ppc64le//usr/share/gocode:/usr/share/gocode"
GORACE=""
GOROOT="/usr/lib/golang"
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_ppc64le"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build777568708=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
go env for s390x:
GOARCH="s390x"
GOBIN=""
GOEXE=""
GOHOSTARCH="s390x"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/builddir/build/BUILDROOT/golang-github-golang-image-0-0.2.20170514.git426cfd8.fc27.s390x//usr/share/gocode:/usr/share/gocode"
GORACE=""
GOROOT="/usr/lib/golang"
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_s390x"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -march=z196 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build232790626=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
What did you do?
Ran the whole x/image test suite on ppc64le and s390x architectures
What did you expect to see?
I expected the whole
x/image
test suite to passWhat did you see instead?
the following test on
golang.org/x/image/vector
failed:The following can be observed in some lines above:
got
want
Which seems to be a precision problem
The text was updated successfully, but these errors were encountered: