[Wasm RyuJit] fix funclet prolog and epilog codegen#126663
[Wasm RyuJit] fix funclet prolog and epilog codegen#126663AndyAyersMS wants to merge 6 commits intodotnet:mainfrom
Conversation
Defer calling genEmitStartBlock until after we've set up the funclet prolog IG. Pass the BasicBlock back from emitter to codegen for the funclet epilog generation since epilog codegen is position dependent.
|
FYI @dotnet/jit-contrib With this plus #126445 codegen for funclets should be in good shape. |
There was a problem hiding this comment.
Pull request overview
This PR fixes WASM RyuJit funclet prolog/epilog emission ordering and makes funclet epilog generation position-aware by passing the associated BasicBlock through to the backend codegen.
Changes:
- Pass the placeholder’s
BasicBlock*fromemitterintoCodeGen::genFuncletEpilog(...)to enable position-dependent epilog emission. - Defer
genEmitStartBlock(block)until after reserving the funclet prolog placeholder IG so the prolog IG is set up correctly (notably for WASM). - Implement WASM funclet epilog emission to choose between
endvsreturnbased on block placement; update other backends for the new signature.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/emit.cpp | Passes igPhBB into genFuncletEpilog(...) during placeholder expansion. |
| src/coreclr/jit/codegen.h | Updates the CodeGen API to genFuncletEpilog(BasicBlock* block). |
| src/coreclr/jit/codegenlinear.cpp | Moves genEmitStartBlock to run after funclet prolog IG reservation. |
| src/coreclr/jit/codegenwasm.cpp | Adds position-dependent WASM funclet epilog emission (end vs return). |
| src/coreclr/jit/codegenxarch.cpp | Updates xarch funclet epilog signature to accept BasicBlock*. |
| src/coreclr/jit/codegenarm.cpp | Updates ARM funclet epilog signature to accept BasicBlock* (unused). |
| src/coreclr/jit/codegenarm64.cpp | Updates ARM64 funclet epilog signature to accept BasicBlock* (unused). |
| src/coreclr/jit/codegenriscv64.cpp | Updates RISC-V64 funclet epilog signature to accept BasicBlock* (unused). |
| src/coreclr/jit/codegenloongarch64.cpp | Updates LoongArch64 funclet epilog signature to accept BasicBlock* (unused). |
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenxarch.cpp:10872
- In the TARGET_AMD64 implementation of genFuncletEpilog, the new
blockparameter is currently unused. On toolchains that enable-Wunused-parameter(often with warnings-as-errors for the JIT), this can fail the build. Either mark it unused (e.g.,BasicBlock* /* block *//UNREFERENCED_PARAMETER(block)) or use it for an assertion/logging like the other backends do.
void CodeGen::genFuncletEpilog(BasicBlock* block)
{
#ifdef DEBUG
if (verbose)
{
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
Realized we shouldn't be allowing methods with EH to fully JIT just yet, as the Wasm we produce won't validate without the JIT host extracting the funclets. So will push another PR here with a bail out, once I've tested it locally. |
unless JitWasmFunclets=1. This is temporary until the JIT host can extract out the funclets as separate Wasm functions.
|
Bail out in place. Updated the top comment. |
| { | ||
| if (block->IsLast() || m_compiler->bbIsFuncletBeg(block->Next())) | ||
| { | ||
| instGen(INS_end); |
There was a problem hiding this comment.
We might want to use a special sentinel opcode instead of a regular end, to make it obvious if one of these somehow slips through the pipeline and ends up in a function body. Then the host can turn it into a regular end.
may make non-consecutive collection calls.
|
FYI @SingleAccretion I went back to LIR flags for now to prevent multiple collections. See #125756 (comment). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenxarch.cpp:10872
blockis unused in this AMD64 implementation, which can trigger-Wunused-parameteron some toolchains and is inconsistent with the other arch implementations that explicitly mark the parameter unused. Either useblock(e.g., for debug dumping) or change the signature toBasicBlock* /* block *// otherwise mark it unreferenced.
void CodeGen::genFuncletEpilog(BasicBlock* block)
{
#ifdef DEBUG
if (verbose)
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/coreclr/jit/regallocwasm.cpp:723
- The comment says we may make multiple collection calls for the same node and only collect once, but the current de-dupe logic only skips a consecutive duplicate (it checks just the previously added node). Consider updating the comment to reflect this (or strengthening the de-dupe if non-consecutive duplicates are possible again).
// We may make multiple collection calls for the same node.
// We only want to collect it once.
//
if (data->m_lastVirtualRegRefsCount > 0)
{
assert(refs != nullptr);
if (node == refs->Nodes[data->m_lastVirtualRegRefsCount - 1])
{
src/coreclr/jit/codegenxarch.cpp:10873
CodeGen::genFuncletEpilog(BasicBlock* block)doesn't useblockon AMD64, which can trigger unused-parameter warnings in some toolchains/configurations. Either mark it as unused (e.g.,BasicBlock* /* block */) or use it (even justassert(block != nullptr)/(void)block).
void CodeGen::genFuncletEpilog(BasicBlock* block)
{
#ifdef DEBUG
if (verbose)
{
printf("*************** In genFuncletEpilog()\n");
Defer calling genEmitStartBlock until after we've set up the funclet prolog IG. Pass the BasicBlock back from emitter to codegen for the funclet epilog generation since epilog codegen is position dependent.
Also, add an on-by default R2R fail-over at the end of codegen for methods with funclets, since the Wasm for method with funclets can't be validated. We can remove this once the JIT host extracts the funclets as separate functions. You can override this via
JitWasmFunclets=1.Also includes a fix for #125756 (comment), which was causing node references to be collected more than once; this would corrupt the node's internal registers.