Fix inaccurate round while being fast #7308
Conversation
This patch applies a clever rounding approach to fix subtle correctness bugs in the implementation of float rounding. It uses copysign to avoid branching, and with the hope of using llvm intrinsics where available. Work in progress, tests are missing. Fixes emscripten-core#7306
Algorithm improvement due to @simonbyrne. Also fixes copy-paste bug from inconsistent variable naming.
| round: function(d) { | ||
| d = +d; | ||
| return d >= +0 ? +Math_floor(d + +0.5) : +Math_ceil(d - +0.5); | ||
| return +Math_trunc(d + +_llvm_copysign_f64(+0.49999999999999994, d)); |
There was a problem hiding this comment.
Note that this is handwritten JS code, and in particular, llvm_copysign_* is present here as a fallback, implementing it in JS when it can't be done otherwise. It's a slow code path, see https://github.com/kripken/emscripten/blob/incoming/src/library.js#L1559
To actually use copysign, this would need to be written in C, perhaps in ./system/lib/libc/extras.c, and it could use a copysign intrinsic there.
There was a problem hiding this comment.
I think the CI failures are because this function is marked as asm.js, and I don't think trunc is a math intrinsic there.
There was a problem hiding this comment.
Yeah, between these two things and f32, it sounds like writing it in C is the better choice.
I could do it here with a ternary and floor, but it would be slightly inferior wasm, with a branch. I'll have a try at the C version.
|
#7306 refers to all sorts of corner cases. It would be good to add tests to the test suite to contain the different corner cases, otherwise the change in this PR is a bit intractable. |
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant. |
This patch applies a clever rounding approach to fix subtle correctness
bugs in the implementation of float rounding. It uses copysign to avoid
branching, and with the hope of using llvm intrinsics where available.
Work in progress, tests are missing.
Fixes #7306