Skip to content

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

Merged
kg merged 1 commit into
dotnet:mainfrom
kg:wasm-jiterpreter-libc-math
Mar 8, 2023
Merged

[wasm] Use libc directly for math in the jiterpreter, implement more opcodes#82963
kg merged 1 commit into
dotnet:mainfrom
kg:wasm-jiterpreter-libc-math

Conversation

@kg

@kgkg commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

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.

@kgkg added arch-wasm WebAssembly architecture area-Codegen-Jiterpreter-mono labels Mar 3, 2023
@ghostghost assigned kgMar 3, 2023
@ghost

ghost commented Mar 3, 2023

Copy link
Copy Markdown

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

kg commented Mar 3, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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

@kg

kg commented Mar 8, 2023

Copy link
Copy Markdown
ContributorAuthor

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
kgforce-pushed the wasm-jiterpreter-libc-math branch from fe91a1a to 7ba4967CompareMarch 8, 2023 17:13
@kg
kg merged commit 14123c9 into dotnet:mainMar 8, 2023
@tannergooding

Copy link
Copy Markdown
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
Copy Markdown
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

kg commented Mar 8, 2023

Copy link
Copy Markdown
ContributorAuthor

@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

kg commented Mar 8, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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

kg commented Mar 8, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

Thanks a ton!

@ghostghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecture

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kg@vargaz@tannergooding