-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Wasm RyuJit] throw helper / null check preliminaries #123053
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 RyuJit] throw helper / null check preliminaries #123053
Conversation
On Wasm null checks must be explicit and exceptions raised via helper call. Set up some of the mechanism we'll need for this: * Add null check special code kind * Track Wasm ACD entries by handler region only (instead of by try or handler). The code address of the helper cannot be used in Wasm to infer EH region containment; we will use use the virtual IP for that. So we need at most one throw helper (per kind) in each funclet and in the main method region. * Ensure throw helper blocks have Wasm labels that are always on the stack in their regions by putting the throw helpers at the end of the region RPO and pretending there is a branch from the region entry. * Add plausible codegen for GT_NULLCHECK
|
FYI @dotnet/jit-contrib (still WIP, need to see how much of this is testable) See #123021 (comment) for some context. |
|
I don't have the code to make |
SingleAccretion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the code to make SCK_NULLCHECK demands in place yet. Seeing as nullchecks get generated in many places it seems less than ideal to track all these sites all down and add code to each one. I wonder if we can just do this in lower or similar.
+1. In fact I don't see the point of this two-phase setup that exists. Why not add all the blocks late (in stack setter or lower) and remove the handling from morph?
I think this is just a long-standing practice that we've never reexamined. Perhaps it is time. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
The inline throw helpers require a bit of coordination since we have to emit a block/end wrapper around the whole thing (so we need to know in advance if we're using throw helpers or not). So I have been working up the divide by zero checks to see how to best generalize the throw helper expansions we'll need. There (whether we use inline or common throw helpers) we need to dup an operand, but Wasm has no Any thoughts on how to best go about having a pool of temps we can use? For the (MinInt/-1) case we need access to the dividend as well. So we might also consider adding these checks explicitly in lower. |
I was thinking we'd reuse the internal register mechanism for that. It's ifdef-ed out currently though, so that will need to be fixed. |
|
@SingleAccretion is this last commit what you had in mind for the unchecked offset? I may push for merging this more or less in its current form and then revisit once #123044 is there to hook up the calls. Still todo, likely again as separate PRs:
|
Yes. Though I think the comparison should be unsigned? |
Yep, changed this. |
|
@dotnet/jit-contrib think this is worth getting in as is, we will need to revisit once some supporting pieces are in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR establishes infrastructure for WebAssembly (Wasm) RyuJIT to handle explicit null checks and exception throwing via helper calls. Unlike other platforms where null checks can be implicit via OS page fault mechanisms, Wasm requires explicit null checks with exceptions raised through helper calls.
Changes:
- Added
SCK_NULL_CHECKspecial code kind for null reference exceptions on Wasm - Modified ACD (Add Code Descriptor) tracking to use handler regions only on Wasm (instead of try or handler regions)
- Ensured throw helper blocks have proper Wasm labels by positioning them at region end and treating them as successors of region entries
- Implemented codegen for
GT_NULLCHECKnodes with both throw helper block and inline variants
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.h | Updated MAX_UNCHECKED_OFFSET_FOR_NULL_OBJECT to 1023 for Wasm (smaller than other platforms) |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Added Wasm-specific logic for maxUncheckedOffsetForNullObject in AOT compiler |
| src/coreclr/jit/gentree.h | Added SCK_NULL_CHECK enum value for null check special code kind |
| src/coreclr/jit/flowgraph.cpp | Added SCK_NULL_CHECK handling throughout exception infrastructure, modified bbThrowIndex for Wasm ACD tracking, fixed typos in comments |
| src/coreclr/jit/stacklevelsetter.cpp | Added GT_NULLCHECK case to register throw helper blocks for Wasm |
| src/coreclr/jit/fgwasm.h | Modified successor enumeration to treat ACD blocks as successors of region entries for proper Wasm control flow |
| src/coreclr/jit/compiler.cpp | Added cross-replay support for Wasm null object offset |
| src/coreclr/jit/codegenwasm.cpp | Implemented null check codegen with comparison against max unchecked offset; added placeholder for divide-by-zero checks |
| src/coreclr/jit/codegen.h | Added inst_JMP overload with isTempLabel parameter for Wasm |
|
@dotnet/jit-contrib PTAL. We are going to use 1024 for the max unchecked offset for now and will decrease it to 0 later on. |
* Trys are no longer required to be contiguous once we've done Wasm layout. * For mutual protect trys, ensure we schedule the handlers from outer to inner so that in the RPO they come out inner to outer * Update the last block of handlers during layout. We don't need to update the last block of trys because they are no longer contiguous.
* Make it clearer when the lack of an ACD block is a missing demand
|
I have a bunch of fixes sitting on top of this, so it would be good if we can get this reviewed. Or else I can add those fixes here... |
|
Merging up to retrigger CI |
|
I pushed some of the follow-on fixes here. This covers most of the flow related stuff seen replaying the x86 crossgen collection. |
kg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple bits around ACDs I don't completely understand but everything looks good.
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
|
/ba-g ongoing issues with arm64 |
On Wasm null checks must be explicit and exceptions raised via helper call.
Set up some of the mechanism we'll need for this: