Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Use libc directly for math in the jiterpreter, implement more opcodes #82963

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

kg
Copy link
Member

@kg kg commented Mar 3, 2023

Previously the jiterpreter used a bunch of hand-written wrappers to call libc math functions. We can export the libc math functions directly and use them that way instead! This makes it easier and less error-prone to implement the remaining math opcodes, so I've done that. I also took this opportunity to use the f32 versions of the libc math functions when appropriate instead of doing promotion/demotion as before.

This shrinks and simplifies the actual generated code for floating point math operations, which should be helpful for large traces that do math.

@kg kg added arch-wasm WebAssembly architecture area-Codegen-Jiterpreter-mono labels Mar 3, 2023
@kg kg requested review from lewing and pavelsavara as code owners March 3, 2023 22:35
@ghost ghost assigned kg Mar 3, 2023
@ghost
Copy link

ghost commented Mar 3, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Previously the jiterpreter used a bunch of hand-written wrappers to call libc math functions. We can export the libc math functions directly and use them that way instead! This makes it easier and less error-prone to implement the remaining math opcodes, so I've done that. I also took this opportunity to use the f32 versions of the libc math functions when appropriate instead of doing promotion/demotion as before.

The trace import section is starting to get pretty bloated, so I'll have to address that in a future PR. But this shrinks and simplifies the actual generated code for floating point math operations, which should be helpful for large traces that do math.

Author: kg
Assignees: -
Labels:

arch-wasm, area-Codegen-Jiterpreter-mono

Milestone: -

@kg
Copy link
Member Author

kg commented Mar 3, 2023

cc @BrzVlad @lambdageek if you have thoughts on a better way to structure this or make sure it matches the interpreter automatically I'd love to hear them, it's easy for errors to creep in since it's still so manual. On the other hand these opcodes probably don't change much

@vargaz
Copy link
Contributor

vargaz commented Mar 8, 2023

Wouldn't exporting all these functions going to cause some problem ?

@kg
Copy link
Member Author

kg commented Mar 8, 2023

Wouldn't exporting all these functions going to cause some problem ?

I'm not sure, the interp already uses them. Do you think it would stop them from being inlined into C callers?

…ppers

Implement more math opcodes and use the f32 functions as appropriate
@kg kg force-pushed the wasm-jiterpreter-libc-math branch from fe91a1a to 7ba4967 Compare March 8, 2023 17:13
@kg kg merged commit 14123c9 into dotnet:main Mar 8, 2023
@tannergooding
Copy link
Member

@kg, we might need to keep a couple of these helpers.

RyuJIT has some cmake based checks in the PAL layer to check for IEEE 754 spec required behavior that the underlying implementations sometimes miss out on: https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/cruntime/math.cpp

I believe mono (and therefore likely WASM) may have (or at least had) some of the managed tests disabled since it wasn't doing the same handling.

@tannergooding
Copy link
Member

-- It's possible these are all fine for WASM, in which case feel free to ignore.

Just wanted to call out the potential consideration.

@kg
Copy link
Member Author

kg commented Mar 8, 2023

@kg, we might need to keep a couple of these helpers.

RyuJIT has some cmake based checks in the PAL layer to check for IEEE 754 spec required behavior that the underlying implementations sometimes miss out on: https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/cruntime/math.cpp

I believe mono (and therefore likely WASM) may have (or at least had) some of the managed tests disabled since it wasn't doing the same handling.

I would be happy to do the work now to make sure we match all of that in a new PR, I've had to make corrections in the past to conform to spec. I'll give that file a look.

The wasm native opcodes for things like min/max have specified behavior that (on my last check) matched spec but I'm not super familiar with 754 so it's possible some of them are wrong. Do we have a conformance suite in tree I could run?

@kg
Copy link
Member Author

kg commented Mar 8, 2023

EDIT: I should also clarify that first and foremost jiterp needs to match the interpreter, otherwise we will get weird bugs. But if the interpreter doesn't match spec that's probably something we should fix too. I had to fix it when adding opcodes for Min and Max, there are dedicated helpers in interp.c for those.

@tannergooding
Copy link
Member

Do we have a conformance suite in tree I could run?

We should generally have managed coverage via the System.Runtime.Extensions tests, such as https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Extensions/tests/System/Math.cs and https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Extensions/tests/System/MathF.cs

We correspondingly have native tests covering various cases here: https://github.com/dotnet/runtime/tree/main/src/coreclr/pal/tests/palsuite/c_runtime

For results that are expected to produce +Inf, +0, -0, or -Inf we typically allow "no variance" and results must be exact. For some others we're allowing a bit more variance than we should due to xplat implementation quirks.

-- If you do the "Institutional Sign In" on https://ieeexplore.ieee.org/Xplore/home.jsp using your @microsoft.com e-mail, you can get access to the IEEE 754:2019 spec which covers particular edge case requirements for many of these functions in 9.2 - Additional mathematical operations (e.g. atan(+infinity) should produce exactly +pi/2). There is a more general expectation that these functions produce an "infinitely precise result, rounded to the nearest representable value" outside the specific edge cases called out (which would ensure xplat determinism); but that's not something we're able to easily achieve today.

-- Many of the same requirements are reiterated in Annex F (normative) IEC 60559 floating-point arithmetic of the C Language specification.

@kg
Copy link
Member Author

kg commented Mar 8, 2023

Thanks! The extensions suite passes with jiterpreter set to aggressive mode, and I can see compile failures for a few math opcodes that aren't implemented right now like coshf, so it seems like things are working OK. I will keep this all in mind if we get any bug reports related to math, and add a to-do item to go over the spec and try to verify how conformant emscripten's libc is.

@tannergooding
Copy link
Member

Thanks a ton!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants