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.

Sign up for GitHub

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

x/image/vector TestMakeFlInXxx fails on s390x and ppc64le #21460

Closed
athos-ribeiro opened this issue Aug 15, 2017 · 7 comments
Closed

x/image/vector TestMakeFlInXxx fails on s390x and ppc64le #21460

athos-ribeiro opened this issue Aug 15, 2017 · 7 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@athos-ribeiro
Copy link

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 pass

What did you see instead?

the following test on golang.org/x/image/vector failed:

go test -compiler gc -ldflags '' golang.org/x/image/vector
--- FAIL: TestMakeFlInXxx (0.00s)
	acc_test.go:473: height 16: got:
		0, 0, -0.022306755, -0.3782405, -0.33334962, -0.1741521, -0.0607556, 0.11660573, 
		0.14664596, 0.4146287, 0.2907673, 0.0001568835, 0, 0, -0.14239307, -0.7012868, 
		-0.15632017, 0, 0, 0, 0, 0, 0, 0.32303032, 
		0.6690931, 0.007876594, -0.05189419, -0.832786, -0.11531975, 0.026225802, 0.42518616, 0.3154636, 
		0.06598757, -0.15304242, -0.47969276, -0.20012793, 0, 0.5327272, 0.46727282, -0.45950258, 
		-0.5404974, 0, 0.63484025, 0.36515972, 0, 0, 0, -0.04351709, 
		-0.82933456, -0.12714837, 0.11087036, 0.88912964, -0.35792422, -0.2053554, 0.0022512944, 0.53743976, 
		0.023588562, 0, 0, 0, 0, -0.55346966, -0.44653034, 0.0002531938, 
		0.9088273, 0.090919495, 0, 0, 0, 0, 0, 0, 
		0, 0, -0.44745448, -0.5525455, 0, 0.80748945, 0.19251058, 0, 
		0, -0.092476256, -0.2661464, -0.11322958, -0.11298219, -0.055094406, 0, -0.16045958, 
		-0.1996116, 0, 0.80748653, 0.19251347, -0.09804727, -0.51129663, -0.3610403, -0.029615778, 
		0, 0, 0, 0, 0, 0, 0, 0.80748653, 
		0.14411622, -0.76251525, -0.1890875, 0.01527351, 0.27528667, 0.29730347, 0.101477206, 0.07259522, 
		0.009900213, -0.34395567, -0.42788061, 0, 0.80748653, -0.3648737, -0.4426128, 0.015778137, 
		0.6826565, 0.30156538, 0, 0, 0, 0, -0.44563293, -0.55436707, 
		0, 0.80748653, -0.60703933, -0.20044717, 0.22371745, 0.77628255, 0, 0, 
		0, 0, 0, -0.44563293, -0.55436707, 0, 0.80748653, -0.75503904, 
		-0.05244744, 0.2797074, 0.7202926, 0, 0, 0, 0, 0, 
		-0.57440215, -0.42559785, 0, 0.80748653, -0.59273535, -0.21475118, 0.04544862, 0.8114854, 
		0.14306602, 0, 0, 0, -0.369642, -0.6184122, -0.011945802, 0, 
		0.7791623, -0.20691396, -0.57224834, 0, 0.08218568, 0.42637306, 0.1586175, -0.089709565, 
		-0.32935485, -0.24788953, -0.00022224105, 0, 0, 0.7085409, 0.2882107, -0.6476579, 
		-0.34909374, 0, 0, 0, 0, 0, 0.16679136, 0.31914654, 
		-0.48593786, 0, 0.537915, 0.462085, -0.00041967133, -0.3120329, -0.41914812, -0.15886839, 
		-0.042683024, 0.19370952, 0.24624406, 0.45803425, -0.07049577, -0.6091341, 0, 0.22253075, 
		want:
		0, 0, -0.022306755, -0.3782405, -0.33334962, -0.1741521, -0.0607556, 0.11660573, 
		0.14664596, 0.41462868, 0.2907673, 0.0001568835, 0, 0, -0.14239307, -0.7012868, 
		-0.15632017, 0, 0, 0, 0, 0, 0, 0.3230303, 
		0.6690931, 0.007876594, -0.05189419, -0.832786, -0.11531975, 0.026225802, 0.42518616, 0.3154636, 
		0.06598757, -0.15304244, -0.47969276, -0.20012794, 0, 0.5327272, 0.46727282, -0.45950258, 
		-0.5404974, 0, 0.63484025, 0.36515975, 0, 0, 0, -0.04351709, 
		-0.8293345, -0.12714837, 0.11087036, 0.88912964, -0.35792422, -0.2053554, 0.0022513224, 0.5374398, 
		0.023588525, 0, 0, 0, 0, -0.55346966, -0.44653034, 0.0002531938, 
		0.9088273, 0.090919495, 0, 0, 0, 0, 0, 0, 
		0, 0, -0.44745448, -0.5525455, 0, 0.80748945, 0.19251058, 0, 
		0, -0.092476256, -0.2661464, -0.11322958, -0.11298219, -0.055094406, 0, -0.16045958, 
		-0.1996116, 0, 0.80748653, 0.19251347, -0.09804727, -0.51129663, -0.3610403, -0.029615778, 
		0, 0, 0, 0, 0, 0, 0, 0.80748653, 
		0.14411622, -0.76251525, -0.1890875, 0.01527351, 0.27528667, 0.29730347, 0.101477206, 0.07259522, 
		0.009900213, -0.34395567, -0.42788061, 0, 0.80748653, -0.3648737, -0.44261283, 0.015778137, 
		0.6826565, 0.30156538, 0, 0, 0, 0, -0.44563293, -0.55436707, 
		0, 0.80748653, -0.60703933, -0.20044717, 0.22371745, 0.77628255, 0, 0, 
		0, 0, 0, -0.44563293, -0.55436707, 0, 0.80748653, -0.7550391, 
		-0.05244744, 0.2797074, 0.72029257, 0, 0, 0, 0, 0, 
		-0.57440215, -0.42559785, 0, 0.80748653, -0.59273535, -0.21475118, 0.04544862, 0.81148535, 
		0.14306602, 0, 0, 0, -0.369642, -0.61841226, -0.011945802, 0, 
		0.7791623, -0.20691396, -0.57224834, 0, 0.08218567, 0.42637306, 0.1586175, -0.089709565, 
		-0.32935485, -0.24788953, -0.00022224105, 0, 0, 0.7085409, 0.28821066, -0.64765793, 
		-0.34909368, 0, 0, 0, 0, 0, 0.16679136, 0.31914657, 
		-0.48593786, 0, 0.537915, 0.462085, -0.00041967133, -0.3120329, -0.41914812, -0.15886839, 
		-0.042683028, 0.19370951, 0.24624406, 0.45803425, -0.07049577, -0.6091341, 0, 0.22253075, 
FAIL
FAIL	golang.org/x/image/vector	0.015s

The following can be observed in some lines above:

got

-0.042683024, 0.19370952, 0.24624406, 0.45803425, -0.07049577, -0.6091341, 0, 0.22253075,

want

-0.042683028, 0.19370951, 0.24624406, 0.45803425, -0.07049577, -0.6091341, 0, 0.22253075,

Which seems to be a precision problem
@gopherbot gopherbot added this to the Unreleased milestone Aug 15, 2017
@mundaym
Copy link
Member

mundaym commented Aug 15, 2017

Thanks for the report. This is almost certainly due to the new fused multiply add (FMA) support on these architectures. We could:

  1. check the outputs are close rather than identical
  2. insert explicit casts to block fused multiply add extraction as needed
  3. add golden data specific to platforms that have FMA enabled

Any thoughts @nigeltao?

@athos-ribeiro athos-ribeiro mentioned this issue Aug 17, 2017
Closed
@nigeltao
Copy link
Contributor

nigeltao commented Sep 1, 2017

I started a golang-dev thread at https://groups.google.com/d/topic/golang-dev/Sti0bl2xUXQ/discussion

@nigeltao
Copy link
Contributor

nigeltao commented Sep 1, 2017

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.

@nigeltao
Copy link
Contributor

nigeltao commented Sep 1, 2017

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.

@gopherbot
Copy link

Change https://golang.org/cl/61024 mentions this issue: vector: prevent fused multiply-add being used in floatingLineTo

@mundaym
Copy link
Member

mundaym commented Sep 4, 2017

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 {floating,fixed}LineTo function only makes up a small portion of profile of the benchmarks I tried though.

@nigeltao
Copy link
Contributor

nigeltao commented Sep 7, 2017

It looks like the floating point code is only really used for testing, is that correct?

Ah, yes, the comment on floatingPointMathThreshold in vector.go says:

// floatingPointMathThreshold is the width or height above which the rasterizer
// chooses to used floating point math instead of fixed point math.
//
// Both implementations of line segmentation rasterization (see raster_fixed.go
// and raster_floating.go) implement the same algorithm (in ideal, infinite
// precision math) but they perform differently in practice. The fixed point
// math version is roughtly 1.25x faster (on GOARCH=amd64) on the benchmarks,
// but at sufficiently large scales, the computations will overflow and hence
// show rendering artifacts. The floating point math version has more
// consistent quality over larger scales, but it is significantly slower.
//
// This constant determines when to use the faster implementation and when to
// use the better quality implementation.
//
// The rationale for this particular value is that TestRasterizePolygon in
// vector_test.go checks the rendering quality of polygon edges at various
// angles, inscribed in a circle of diameter 512. It may be that a higher value
// would still produce acceptable quality, but 512 seems to work.
const floatingPointMathThreshold = 512

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.

@golang golang locked and limited conversation to collaborators Sep 7, 2018
mrhyperbit23z0d added a commit to mrhyperbit23z0d/bhegde8 that referenced this issue Jun 6, 2022
@mrhyperbit23z0d
vector: prevent fused multiply-add being used in floatingLineTo
a4c3ff6 
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>
GalaxyForcew added a commit to GalaxyForcew/A1bisshy that referenced this issue Jun 6, 2022
@GalaxyForcew
vector: prevent fused multiply-add being used in floatingLineTo
da8f7a7 
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>
yi-ge3 added a commit to yi-ge3/wislie that referenced this issue Jun 6, 2022
@yi-ge3
vector: prevent fused multiply-add being used in floatingLineTo
f9dd221 
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>
balloontmz6 added a commit to balloontmz6/Likewise42l that referenced this issue Jun 6, 2022
@balloontmz6
vector: prevent fused multiply-add being used in floatingLineTo
072b886 
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>
snapbakkhfbav added a commit to snapbakkhfbav/SayedBaladohr that referenced this issue Oct 6, 2022
@snapbakkhfbav
vector: prevent fused multiply-add being used in floatingLineTo
99584b8 
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>
MiderWong5ddop added a commit to MiderWong5ddop/sidie88f that referenced this issue Oct 7, 2022
@MiderWong5ddop
vector: prevent fused multiply-add being used in floatingLineTo
a9b617f 
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>
rorypeckwnt4v added a commit to rorypeckwnt4v/LearnByBhanuPrataph that referenced this issue Oct 7, 2022
@rorypeckwnt4v
vector: prevent fused multiply-add being used in floatingLineTo
75e47be 
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>
egorovcharenko9 added a commit to egorovcharenko9/RiceBIOC470z that referenced this issue Oct 7, 2022
@egorovcharenko9
vector: prevent fused multiply-add being used in floatingLineTo
a4c65ba 
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>
RafayGhafoorf added a commit to RafayGhafoorf/dustinsand8 that referenced this issue Oct 7, 2022
@RafayGhafoorf
vector: prevent fused multiply-add being used in floatingLineTo
9a669af 
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@athos-ribeiro @nigeltao @gopherbot @mundaym