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

x/exp/shiny/iconvg: Encoder.SetEllipticalGradient floating point math diverges on ARM64 due to FMA #43219

Closed
bcmills opened this issue Dec 16, 2020 · 8 comments
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 16, 2020

2020-12-15T15:35:30-b5a6e24/darwin-arm64-11_0-toothrot

--- FAIL: TestEncodeElliptical (0.00s)
    encode_test.go:48: 
        got  88 bytes:
        89 49 56 47 00 98 02 8a ca 00 0a 4a ae af aa aa bc bd 0a ac 03 00 00 b2 ab 8b 88 08 3d aa 00 b9 a0 87 4b af 00 87 03 af 02 00 40 c0 40 40 e6 c0 e8 c0 e6 40 e1 80 7c c0 56 6c 02 58 6a 5a 6c 58 6e e1 c0 56 9c 02 58 9a 5a 9c 58 9e e1 c0 92 8a 02 94 88 96 8a 94 8c e1
        want 85 bytes:
        89 49 56 47 00 98 02 8a ca 00 0a 4a ae af aa aa bc bd 0a ac 00 ab 8b 88 08 3d aa 00 b9 a0 87 4b af 00 87 03 af 02 00 40 c0 40 40 e6 c0 e8 c0 e6 40 e1 80 7c c0 56 6c 02 58 6a 5a 6c 58 6e e1 c0 56 9c 02 58 9a 5a 9c 58 9e e1 c0 92 8a 02 94 88 96 8a 94 8c e1
FAIL
FAIL	golang.org/x/exp/shiny/iconvg	0.953s

CC @golang/release @nigeltao @cherrymui

@bcmills bcmills added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-arm64 labels Dec 16, 2020
@gopherbot gopherbot added this to the Unreleased milestone Dec 16, 2020
@cherrymui
Copy link
Member

cherrymui commented Dec 16, 2020

Same error on linux/arm64 (which doesn't seem to run x/exp tests on the dashboard).

Without looking into the detail, my guess is that it has something to do with how floating points are handled on different architectures. I'll look into it.

@cherrymui
Copy link
Member

With some manual decoding, it in the end boils down to this line (https://go.googlesource.com/exp/+/refs/heads/master/shiny/iconvg/encode.go#419)

mc := -ma*cx - mb*cy

On AMD64 it results in

mc=-(-0.020833334)*(-20)-(0.041666668)*(-10)=0

On ARM64,

mc=-(-0.020833334)*(-20)-(0.041666668)*(-10)=-7.450581e-09

which leads to different encoding.

On ARM64, it generates

v5  00004 (+13) FMOVS "".ma(SB), F0
v8  00005 (13) FMOVS "".cx(SB), F1
v9  00006 (13) FNMULS F1, F0, F0
v11 00007 (13) FMOVS "".mb(SB), F1
v13 00008 (13) FMOVS "".cy(SB), F2
v15 00009 (13) FMSUBS F1, F0, F2, F0

with a fused multiply-sub instruction.

If we add explicit rounding to prevent fused operations,

mc := -float32(ma*cx) - float32(mb*cy)

then it passes on ARM64.

Without the explicit rounding, I think it is allowed by the spec for the compiler to generate fused instructions, which will result in slightly different values. So, maybe the test shouldn't test exact values for floating points, or, if it really wants to, add the explicit rounding.

@dmitshur
Copy link
Contributor

dmitshur commented Dec 18, 2020

Thanks for investigating Cherry.

The test expects byte-identical encoding, which we now see isn't going to be the case across architectures.

Package iconvg documentation states:

It is a format for efficient presentation, not an authoring format. [...] It is not a pixel-exact format. Different implementations may produce slightly different renderings, due to implementation-specific rounding errors in the mathematical computations when rasterizing vector paths to pixels.

I understand that should imply that it's not a goal for iconvg to try to guarantee byte-exact encoding across architectures, and so the test should be fixed. Please let me know if you prefer we take the other path and do try to make encoding deterministic. CC @nigeltao.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 18, 2020
@nigeltao
Copy link
Contributor

I understand that should imply that it's not a goal for iconvg to try to guarantee byte-exact encoding across architectures,

Decoding (from IconVG to pixels) is not expected to be byte-exact (or RGBA-exact), as there are multiple vector rasterization algorithms of varying speed-quality trade-offs.

However, when encoding (to IconVG), I would prefer that e.g. a "convert SVG to IconVG" tool had identical output for identical SVG input, regardless of the GOARCH. I would prefer an explicit

mc := -float32(ma*cx) - float32(mb*cy)

together with a comment similar to https://github.com/golang/image/blob/35266b937fa69456d24ed72a04d75eb6857f7d52/vector/raster_floating.go#L58-L68

I'm not confident in making the CL myself, since I don't have easy access to an ARM64 system right now to make sure that all the broken tests would be fixed, but I'm happy to review a CL.

@dmitshur
Copy link
Contributor

dmitshur commented Dec 19, 2020

@nigeltao Sounds good, thanks for feedback. I’ll send a CL that documents that goal and fixes the implementation accordingly.

@dmitshur dmitshur removed OS-Darwin Testing An issue that has been verified to require only test changes, not just a test failure. labels Dec 19, 2020
@dmitshur dmitshur changed the title x/exp/shiny/iconvg: TestEncodeElliptical failing on darwin-arm64-11_0-toothrot builder x/exp/shiny/iconvg: Encoder.SetEllipticalGradient floating point math diverges on ARM64 due to FMA Dec 19, 2020
@dmitshur dmitshur self-assigned this Dec 19, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Dec 20, 2020

when encoding (to IconVG), I would prefer that e.g. a "convert SVG to IconVG" tool had identical output for identical SVG input, regardless of the GOARCH.

That said, I do want to ask, how much confidence can we have in our ability to achieve and maintain this goal in the general case (that is, for all specification-compliant Go implementations, including compilers other than gc)?

Considering the room the Go specification leaves to implementation (e.g., "In all non-constant conversions involving floating-point or complex values, if the result type cannot represent the value the conversion succeeds but the result value is implementation-dependent.") and various articles like https://randomascii.wordpress.com/2013/07/16/floating-point-determinism/, https://gafferongames.com/post/floating_point_determinism/, I haven't been able to convince myself yet that floating-point determinism can be achieved reliably in a forward-compatible way.

I think there's no harm to try it for now, and reconsider in the future if we encounter sufficient evidence that shows it's not sustainable to guarantee IconVG's deterministic-encoding property (due to it involving floating-point computations). But I wanted to ask in case you have more thoughts on this topic.

@gopherbot
Copy link

Change https://golang.org/cl/279294 mentions this issue: shiny/iconvg: make SetEllipticalGradient math deterministic

@nigeltao
Copy link
Contributor

I'll defer to @cherrymui re Go floating-point determinism and FMA, as I think it's more of a Go compiler question than an x/exp/shiny or x/image/vector question.

I agree to try it for now and reconsider if it's not working.

@golang golang locked and limited conversation to collaborators Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants