Skip to content

[Wasm RyuJit] fix funclet prolog and epilog codegen#126663

Open
AndyAyersMS wants to merge 6 commits intodotnet:mainfrom
AndyAyersMS:WasmFuncletCodegen1
Open

[Wasm RyuJit] fix funclet prolog and epilog codegen#126663
AndyAyersMS wants to merge 6 commits intodotnet:mainfrom
AndyAyersMS:WasmFuncletCodegen1

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

@AndyAyersMS AndyAyersMS commented Apr 8, 2026

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.

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.
Copilot AI review requested due to automatic review settings April 8, 2026 21:37
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 8, 2026
@AndyAyersMS
Copy link
Copy Markdown
Member Author

FYI @dotnet/jit-contrib

With this plus #126445 codegen for funclets should be in good shape.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 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* from emitter into CodeGen::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 end vs return based 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 block parameter 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)
    {

@am11 am11 added the arch-wasm WebAssembly architecture label Apr 8, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

@AndyAyersMS
Copy link
Copy Markdown
Member Author

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.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

Bail out in place. Updated the top comment.

{
if (block->IsLast() || m_compiler->bbIsFuncletBeg(block->Next()))
{
instGen(INS_end);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copilot AI review requested due to automatic review settings April 9, 2026 00:15
@AndyAyersMS
Copy link
Copy Markdown
Member Author

FYI @SingleAccretion I went back to LIR flags for now to prevent multiple collections. See #125756 (comment).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • block is unused in this AMD64 implementation, which can trigger -Wunused-parameter on some toolchains and is inconsistent with the other arch implementations that explicitly mark the parameter unused. Either use block (e.g., for debug dumping) or change the signature to BasicBlock* /* block */ / otherwise mark it unreferenced.
void CodeGen::genFuncletEpilog(BasicBlock* block)
{
#ifdef DEBUG
    if (verbose)
    {

Copilot AI review requested due to automatic review settings April 9, 2026 16:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 use block on AMD64, which can trigger unused-parameter warnings in some toolchains/configurations. Either mark it as unused (e.g., BasicBlock* /* block */) or use it (even just assert(block != nullptr) / (void)block).
void CodeGen::genFuncletEpilog(BasicBlock* block)
{
#ifdef DEBUG
    if (verbose)
    {
        printf("*************** In genFuncletEpilog()\n");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants