Fused Multiply Add (FMA) inconsistency in unconvert output and golang.org/x/image/vector code

[dmitshur]

unconvert is expected to be FMA-aware as of commit 1a9a0a0, yet I've discovered the following inconsistency between its current behavior, and an internal comment written in the golang.org/x/image/vector package.

I suspect it's either a false-positive problem in unconvert, or an opportunity to simplify golang.org/x/image/vector code without actually changing behavior. At this time, I am not sure which of the two it is, and so this issue report may be in the wrong location. If so, I apologize about that. I opened an issue here to start the conversation, but I'm happy to move the issue if it's determined not to be a problem in unconvert.

The output of running the latest version of unconvert (commit 5ecdfa8) on the latest version of golang.org/x/image/vector (commit golang/image@cd38e80) is:

$ unconvert golang.org/x/image/vector
/gopath/src/golang.org/x/image/vector/raster_floating.go:75:15: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:91:22: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:97:17: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:99:17: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:102:22: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:107:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:110:18: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:112:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:114:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:122:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:127:22: unnecessary conversion

With the default false value of -fastmath flag (i.e., do not remove conversions that force intermediate rounding), unconvert finds 11 instances of unnecessary conversions.

When -fastmath=true is set, it finds 15 instances:

Output with -fastmath=true Flag

$ unconvert -fastmath=true golang.org/x/image/vector
/gopath/src/golang.org/x/image/vector/raster_floating.go:69:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:75:15: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:86:18: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:88:26: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:91:22: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:97:17: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:99:17: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:102:22: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:107:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:110:18: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:112:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:114:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:120:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:122:23: unnecessary conversion
/gopath/src/golang.org/x/image/vector/raster_floating.go:127:22: unnecessary conversion

That means unconvert considers 11 to be unnecessary, and 4 necessary (for FMA purposes).

However, an internal comment in the raster_floating.go currently says:

// The "float32" in expressions like "float32(foo*bar)" here and below
// look redundant, since foo and bar already have type float32, but are
// explicit in order to disable the compiler's Fused Multiply Add (FMA)
// instruction selection, which can improve performance but can result
// in different rounding errors in floating point computations.
//
// This package aims to have bit-exact identical results across all
// GOARCHes, and across pure Go code and assembly, so it disables FMA.
//
// See the discussion at
// https://groups.google.com/d/topic/golang-dev/Sti0bl2xUXQ/discussion

@mdempsky Do you believe that the output of unconvert is accurate in this particular case, and those conversions are indeed unnecessary, despite what the comment may suggest?

/cc @mundaym and @nigeltao who created and reviewed golang/image@e20db36, which added that comment and the explicit float32 expressions to disable FMA selection in golang.org/x/image/vector package.

[mdempsky]

An implementation may combine multiple floating-point operations into a single fused operation, possibly across statements, and produce a result that differs from the value obtained by executing and rounding the instructions individually. A floating-point type conversion explicitly rounds to the precision of the target type, preventing fusion that would discard that rounding.

unconvert's logic about handling floating point conversions is not aware of the "possibly across statements" clause. It's only aware of conversions of the form T(x op y) op z (or x op T(y op z)).

It should probably treat conversions to floating point type as always necessary, except when -fastmath=true.