Wasm: Implement fmod on the Scala side. #5149
Conversation
In our IR, the rhs of Long shifts is an Int, but in Wasm it must be a Long as well. In the common-case where the rhs is a constant, we now constant-fold the extension to 64 bits.
gzm0
left a comment
gzm0
left a comment
There was a problem hiding this comment.
Nits and suggestions. Barring major roadblocks for using raw double to long bits, this can be merged IMO.
| test(Double.NaN, Double.NaN, 5.5) | ||
| test(Double.NaN, Double.NaN, -151.189) | ||
| test(Double.NaN, Double.NaN, 3.123e-320) | ||
| test(Double.NaN, Double.NaN, 2.253547e-318) |
There was a problem hiding this comment.
Could you add an explanation to the commit message what these additional values are?
| var iy: IType = toBits(y) | ||
|
|
||
| // let sx = ix & F::SIGN_MASK; | ||
| val sx: IType = ix & SignBit |
There was a problem hiding this comment.
Nit: Move below ex / ey for consistent ordering with original code?
There was a problem hiding this comment.
We have to do it here because we're masking off the sign bits. I added a comment.
| var ey: Int = wrapToInt(iy >>> mbits) | ||
|
|
||
| /* if iy << 1 == zero || y.is_nan() || ex == F::EXP_SAT as i32 { | ||
| * return (x * y) / (x * y); -> sent to after the block |
There was a problem hiding this comment.
I do not understand what "sent to after the block" means here. Obsolete?
| * | ||
| * I believe the first one is just wrong, and fails to correctly handle | ||
| * subnormal values. Removing that one causes the tests for subnormals to | ||
| * fail. |
There was a problem hiding this comment.
I was going to suggest contributing upstream, but it seems there are not unit tests for fmod at all upstream :-/ (or I couldn't find them...)
There was a problem hiding this comment.
I "contributed" a review upstream: rust-lang/libm#469 (review)
So far the author agrees with my analysis, although they haven't had a chance to integrate my suggestion yet.
There was a problem hiding this comment.
Thank you for the consideration! I'm looking into this at rust-lang/libm#535.
| @inline def extendFromInt(v: Int): IType = v.toLong | ||
| @inline def wrapToInt(v: IType): Int = v.toInt | ||
|
|
||
| // --- BEGIN COPY-PASTE wrt. fmodf ---------------------------------------- |
There was a problem hiding this comment.
Add a comment that copy-pasting here is better than an @inline helper because of number operations? (<<, etc.)?
| val iZero = 0L | ||
| val iOne = 1L | ||
|
|
||
| @inline def toBits(v: FType): IType = java.lang.Double.doubleToLongBits(v) // TODO use raw variant |
There was a problem hiding this comment.
Do we have a strategy to solve this? Feels like the answer to that is relevant to decide between this PR and #5147?
There was a problem hiding this comment.
Using the non-raw variant is correct. Using the raw variant would only be more efficient. (I expanded the comment.)
This avoids the JS call overhead for that "primitive" operation. As can be seen in the implementation, `fmod` is far from a primitive, though. It requires an involved algorithm in software. We took an MIT implemention of `fmod` written in Rust, generic in the bit-width, and translated it to Scala. The "generic" aspect is turned into a big copy-pasted blob between the `Float` and `Double` versions, with some "configuration" at the top. We put that implementation in the linker private library, in a new object `WasmRuntime`. The Wasm backend compiles `Float_%` and `Double_%` as calls to those runtime functions. Since our new implementation is sensitive to the normal/subnormal distinction, we add new tests for subnormal values.
3 participants
This avoids the JS call overhead for that "primitive" operation.
As can be seen in the implementation,
fmodis far from a primitive, though. It requires an involved algorithm in software.We took an MIT implemention of
fmodwritten in Rust, generic in the bit-width, and translated it to Scala. The "generic" aspect is turned into a big copy-pasted blob between theFloatandDoubleversions, with some "configuration" at the top.We put that implementation in the linker private library, in a new object
WasmRuntime. The Wasm backend compilesFloat_%andDouble_%as calls to those runtime functions.(Probably better) alternative to #5147. At least we read Scala code and not Wasm code. It's not a lot less code in total, but there are 175 lines that are a brutal copy-paste, so those duplicate 175 lines don't need to be reviewed.