Wasm call fixes and more

[AndyAyersMS]

• GT_BOUNDS_CHECK codegen
• GT_KEEPALIVE codegen
• Actually emit helper calls
• Fix GT_PUTARG assert in delegate invoke lowering
• Fix arity check/arg fetch in morph's ARRADDR_ST optimization
• Fix arity issues in VN for helper calls

[Copilot]

Pull request overview

This PR fixes multiple issues related to WebAssembly call handling and JIT compilation. The changes address argument indexing problems caused by the WasmShadowStackPointer argument, implement missing codegen for bounds checks and keepalive operations, and enable proper helper call emission.

Changes:

• Fixes argument indexing in value numbering and morphing to account for non-user arguments like WasmShadowStackPointer
• Implements GT_BOUNDS_CHECK and GT_KEEPALIVE codegen for WebAssembly
• Enables actual helper call emission instead of placeholder TODOs
• Attempts to fix delegate invoke lowering for platforms without fixed register sets

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/coreclr/jit/valuenum.cpp	Changes GetArgByIndex to GetUserArgByIndex to skip non-user arguments; adds logic to skip WasmShadowStackPointer in VN processing
src/coreclr/jit/morph.cpp	Moves WASM stack pointer argument addition to correct location; fixes ARRADDR_ST optimization to use CountUserArgs/GetUserArgByIndex
src/coreclr/jit/lower.cpp	Adds conditional compilation for delegate invoke lowering based on HAS_FIXED_REGISTER_SET (contains critical bugs)
src/coreclr/jit/codegenwasm.cpp	Implements GT_BOUNDS_CHECK and GT_KEEPALIVE codegen; enables helper call emission for null checks and bounds checks

[AndyAyersMS]

fyi @dotnet/jit-contrib

One general theme here is fallout from having managed calls pass the Wasm Shadow SP as first arg. A number of places in the JIT assume they know what certain call args represent by position. This is generally no longer true for Wasm. I am fairly sure more GetArgByIndex calls need to change to GetUserArgByIndex but am thinking perhaps we should also enforce the convention that all non-user args are well-known args, and that fetching a well-known arg should primarily rely on FindWellKnownArg (or if fetching a well-known arg by index, we should verify we got the one we expected).

Since the SP arg is added at morph we currently need to know if a piece of code is running before or after morph, and whether or not the call has managed calling convention before we can decide if there should be an SP arg or not...

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/jit/codegenwasm.cpp:1224

• This code incorrectly emits a comparison that duplicates the comparison already performed by the caller. Both genCodeForNullCheck (lines 1246-1247) and genRangeCheck (lines 1260-1261) emit their own comparison instructions before calling genJumpToThrowHlpBlk. The comparison result is already on the stack when genJumpToThrowHlpBlk is called, so these lines should be removed. The function should expect the comparison result to be on the stack and only emit the conditional jump (inst_JMP with EJ_jmpif on line 1225).

        GetEmitter()->emitIns_I(INS_I_const, EA_PTRSIZE, m_compiler->compMaxUncheckedOffsetForNullObject);
        GetEmitter()->emitIns(INS_I_le_u);

[dotnet-policy-service]

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

[AndyAyersMS]

@dotnet/jit-contrib anyone up for a review?

[AndyAyersMS]

/ba-g infrastructure issues

[SingleAccretion]

LGTM modulo one comment.

It is nice to see these codegen functions like genRangeCheck being so simple.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.