Fix wasm R2R JIT validation issues#129555
Conversation
- Use call node type when forming wasm call signatures. - Treat GT_FRAME_SIZE as a wasm leaf during use-edge iteration. - Classify single-register struct call stores by wasm ABI type. - Preserve ABI segment widths for wasm parameter register remaps. - Bail out on wasm method/call signatures over the engine limit. Issue: dotnet#129497 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@adamperlin PTAL |
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adjusts WebAssembly (Wasm) JIT/R2R codegen plumbing to produce wasm-valid call/method signatures and to avoid validation/assert failures in several JIT phases when compiling for Wasm.
Changes:
- Update Wasm call signature construction and add explicit bailouts when a call/function would exceed the Wasm engine parameter limit (1000).
- Treat
GT_FRAME_SIZEas a leaf for Wasm during use-edge iteration to avoid consumers assuming it has operands. - Refine Wasm ABI typing in lowering for single-register struct call stores and preserve ABI segment widths when inducing parameter-register locals.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lower.cpp | Adjusts Wasm ABI-based typing for single-reg struct call stores; preserves register-segment type widths for induced parameter locals. |
| src/coreclr/jit/lclvars.cpp | Adds a Wasm-specific compilation bailout when the function argument count exceeds the Wasm engine limit. |
| src/coreclr/jit/gentree.cpp | Treats GT_FRAME_SIZE as a leaf for Wasm in GenTreeUseEdgeIterator. |
| src/coreclr/jit/error.h | Introduces WASM_IMPL_LIMITATION macro (Wasm-only impl-limitation helper). |
| src/coreclr/jit/compiler.h | Defines MaxWasmFunctionParameters constant for Wasm. |
| src/coreclr/jit/codegenwasm.cpp | Adjusts wasm call signature construction and adds a call-argument-count bailout prior to requesting a type symbol. |
| // Compute the wasm-level result type for the call. | ||
| const var_types callRetType = genActualType(call); |
There was a problem hiding this comment.
Sure enough, but for normal calls this is the right logic. Will revise.
| #ifdef TARGET_WASM | ||
| #define WASM_IMPL_LIMITATION(msg) IMPL_LIMITATION(msg) | ||
| #else | ||
| #define WASM_IMPL_LIMITATION(msg) | ||
| #endif // TARGET_WASM |
| #ifdef TARGET_WASM | ||
| #define WASM_IMPL_LIMITATION(msg) IMPL_LIMITATION(msg) | ||
| #else | ||
| #define WASM_IMPL_LIMITATION(msg) | ||
| #endif // TARGET_WASM |
| #ifdef TARGET_WASM | ||
| class WasmInterval; // defined in fgwasm.h | ||
| enum class WasmValueType : unsigned; | ||
| constexpr unsigned MaxWasmFunctionParameters = 1000; |
There was a problem hiding this comment.
Is this due to the "implementation limitation" dynamic cap? There are many other such caps, not all of them you can circumvent by rejecting the method (e. g. total number of methods in a WASM image).
It doesn't seem worthwhile to me to try to implement fallbacks for caps only because they're hit in our test code.
For NAOT, rejecting the compilation would mean creating an always-throw body, which would obscure the callee cases diagnostics-wise.
There was a problem hiding this comment.
My understanding is it's due to the implementation limitation. Perhaps we can open an issue to revisit this for some of the other caps, in particular for NativeAOT? @AndyAyersMS can you possibly link to the V8 implementation limits as a comment? I see the max arg limit for v8 is defined here: https://github.com/v8/v8/blob/13edc96200c471c3789f9a87f278d0f382c1e216/src/wasm/wasm-limits.h#L51
There was a problem hiding this comment.
Perhaps we can open an issue to revisit this for some of the other caps, in particular for NativeAOT?
To be clear, my main argument is not that it's difficult (although it is) to work around (some of) these caps, but that there should be some stronger reason for doing so. We have (something like) a limit of 65k params on x86 currently, but it's not a problem because practical functions don't have 65k params. Do we have practical functions with 1k params?
The bail out is incomplete; crossgen2 will need similar fixes.
Issue: #129497