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

[bcmills]

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

[cherrymui]

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]

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]

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.

[nigeltao]

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]

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