Add --fast-math mode #3155
Add --fast-math mode #3155
Conversation
|
Could |
|
@MaxGraey yes, it may make sense to have something more fine-grained eventually. @sunfishcode mentioned that this is what clang and gcc have, with separate flags for NaNs and such. I think that's a reasonable thing to aim for. For now, I think this PR is a good first step. It fixes fuzzer failures, gets us back to emitting correct code, and avoids just ripping out the relevant optimizations, instead putting them behind a flag. |
| @@ -1369,7 +1372,8 @@ struct OptimizeInstructions | |||
| } | |||
| { | |||
| double value; | |||
| if (matches(curr, binary(Abstract::Sub, any(), fval(&value))) && | |||
| if (fastMath && | |||
| matches(curr, binary(Abstract::Sub, any(), fval(&value))) && | |||
| value == 0.0) { | |||
| // x - (-0.0) ==> x + 0.0 | |||
There was a problem hiding this comment.
What's wrong about this optimization without fastMath? Is it that it doesn't change non-canonical NaN bits but it should?
There was a problem hiding this comment.
Hmm, actually, re-reading it now I'm not sure. It doesn't remove a math operation (unlike the others), so it would still change NaNs as expected, I think? I guess we'd need to read the spec (wasm? IEEE?) carefully. If no one knows offhand, the safe thing may be to land this with a TODO for later.
There was a problem hiding this comment.
Imteresting article:
https://754r.ucbtest.org/background/nan-propagation.pdf
See "What should happen when two payloads are combined?"
There was a problem hiding this comment.
And yes seems to be IEEE754 doesn't specify how two NaNs with different payloads should be combined. I guess it should be reflect on wasm spec tests
There was a problem hiding this comment.
Btw webassembly spec specify Na N-propogation rules for fneg, fabs and fcopysign:
https://webassembly.github.io/spec/core/bikeshed/index.html#nan-propagation%E2%91%A0
| // and assuming math follows the algebraic rules for associativity and so | ||
| // forth (which IEEE floats do not, strictly speaking). This is inspired by | ||
| // gcc/clang's -ffast-math flag. | ||
| bool fastMath = false; |
There was a problem hiding this comment.
"Fast math" has many meanings, including ignoring negative zero, ignoring infinities, ignoring NaNs, ignoring signaling NaN, allowing for greater precision, and allowing for reduced precision. GCC and clang have moved to have several different flags for these things, as no single definition of "fast math" works for everyone. I encourage Binaryen to follow GCC and clang here.
There was a problem hiding this comment.
Agreed, @sunfishcode , that is the plan. This is just the first step.
There was a problem hiding this comment.
What is the status of this plan?
There was a problem hiding this comment.
@sunfishcode So far the use cases for the fast math flag have only been ignoring NaNs, AFAIK. So we've not added more specific flags. I imagine we will when we start to optimize them.
Do you have more use cases or ideas perhaps?
There was a problem hiding this comment.
With more levels we could do more floating points optimizations:
#3155 (comment)
There was a problem hiding this comment.
A separate ignoreNaNs flag, which could also be enabled by fastMath, would allow users that want to ignore NaNs do so without having to know this implementation detail about Binaryen, and without opting into unknown optimizations in future versions of Binaryen.
There was a problem hiding this comment.
Makes sense.
Is this something you'd use soon @sunfishcode ? If so I can open a PR later today probably. (Or maybe @MaxGraey you'd want to?) If it's not urgent we could open an issue so we don't forget.
There was a problem hiding this comment.
@kripken If this PR only add separate ignoreNaNs flags then I think it would be better that you open PR. I might do a more substantial PR adding a few levels for fastMath a bit later.
There was a problem hiding this comment.
Ok, I found some time, PR up: #4262
Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>
|
Added the review suggestion change, and the fuzzer meanwhile found another case I missed, c4cfcba |
Similar to clang and gcc,
--fast-mathmakes us ignore corner cases of floating-pointmath like NaN changes and (not done yet) lack of associativity and so forth.
This undoes some changes (#2958 and #3096) where we assumed it was
ok to not change NaN bits, but @binji corrected us. We can only do such things in fast
math mode. This puts those optimizations behind that flag, adds tests for it, and
restores the interpreter to the simpler code from before with no special cases.