-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Wasm RyuJIT] Initial scaffolding for calls and helper calls #123044
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
741ddb8 to
e805a10
Compare
AndyAyersMS
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.
We still need to figure out in detail how to specify what method we want to call.
We know roughly what kind of Wasm we want to produce (per the calling convention doc we'll be making lots of indirect calls) but we need help from the JIT host to make this all work out.
I think it might be ok for now to just defer this part of the work and try and get everything else lined up?
| // Generate a direct call to a non-virtual user defined or helper method | ||
| assert(call->IsHelperCall() || (call->gtCallType == CT_USER_FUNC)); | ||
|
|
||
| if (call->gtEntryPoint.addr != NULL) |
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.
Per the calling convention, for user and helper calls we'll also be invoking them indirectly.
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.
Right. I'm not clear on whether that means there won't be an addr filled in
b95ce18 to
499127d
Compare
|
This branch now hits this in crossgen2: |
|
For this: [MethodImpl(MethodImplOptions.NoInlining)]
static void voidFunc () {
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void callVoidFunc () {
voidFunc();
}R2R now compiles without crashing or asserting, and disasmo generates this: ; Method Program:callVoidFunc() (FullOpts)
G_M45341_IG01: ;; offset=0x0000
local.cnt 4
local[0] type=i32
local[1] type=i64
local[2] type=f32
local[3] type=f64
;; size=9 bbWeight=1 PerfScore 5.00
G_M45341_IG02: ;; offset=0x0009
i32.const 140722950757336
i32.load 0 0
call_indirect 0 0
;; size=14 bbWeight=1 PerfScore 3.00
G_M45341_IG03: ;; offset=0x0017
end
;; size=1 bbWeight=1 PerfScore 1.00
; Total bytes of code: 24It's not correct but it's a start. |
|
cc @dotnet/jit-contrib need to figure out how this should actually be factored and shaped and implemented, now that it "works". I don't understand 75% of the code I touched here, for example whether we use EA_8BYTE or a different one still makes no sense to me, and the way we historically separate out |
|
@kg what do we need to do to unblock this? I don't think we'll have the crossgen2 relocation stuff around for a bit, so emitting long-form placeholder values for the various indices/offsets is the best we can do. If you want to try getting the reloc machinery started with this PR then we can look into that too. |
As far as I'm concerned it's done, but I was waiting for someone to review it on the presumption that there's a bunch of things wrong in this PR since I didn't really know what I was doing. |
We should be able to do some testing, especially now that #123262 is in. What happens if we try and compile a method that has a call (eg |
Is this what you mean? #123044 (comment) |
src/coreclr/jit/emit.h
Outdated
| EC_MANAGED_PTR, // call_indirect to an unknown managed method or helper (two levels of indirection - ptr to | ||
| // arbitrary cell) | ||
| // should generate: <push args>; call <func-index> | ||
| EC_FUNCTION_INDEX, // call to a known wasm import or function from same module, no indirection |
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.
For WASM we only have direct call and indirect call_indirect, which already map to EC_FUNC_TOKEN and EC_INDIR_R, so I don't think we need these kinds to be under ifdefs.
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.
We don't have the concept of function pointers in wasm? Or is the idea that the extra level of indirection will be handled somewhere else?
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.
The proposal is to use EC_FUNC_TOKEN for what is currently called EC_FUNCTION_INDEX, remove EC_MANAGED_CELL and EC_MANAGED_PTR by handling the indirections they represent in the caller(s), and using EC_INDIR_R to represent "emit call_indirect".
The overall principle is to have emitIns_Call only emit one WASM instruction, the call itself. It is (more) consistent with how other targets represent these things.
Yes --make sure that basic cases look plausible |
774a027 to
3c2e14c
Compare
|
@kg somehow my comments never got posted, so some are out of date. At any rate I think we can just bypass calling |
|
FindEarliestPutArg also appears to call MarkCallPutArgAndFieldListNodes. I will try disabling legalization. |
That's true, but only because we don't have any stack arguments currently (were we to have them, we'd have |
With legalization disabled: I think we can keep chasing all these down, but is it strictly necessary to get rid of putarg? What's bad about having it, at least for now? |
This is weird, according to the spew the constant is 0x420040 but we're seemingly getting a huge 64-bit constant (that I'm dutifully writing out for now). Or is that huge number the r2r cell address? |
|
In codegenwasm.cpp: It looks like we have TYP_INT constants with garbage in the high bits somehow. |
|
I think the problem is here: The value in |
|
The right long-term fix is to mark (the instr desc for) this constant as needing a reloc, and have the instr desc refer to the (64 bit) compile-time handle, and encode a maximum-length value in the constant field, and then have the emitter call The value we write into the constant can potentially be anything since it will be replaced by the reloc later on. Likely zero is a good choice, or perhaps the low 32 bits of the compile time handle. For now you might just leave a todo for the reloc part and encode zero. |
fc8c8bd to
83bc9d1
Compare
|
Latest failure is outside crossgen in the test harness: Two of those missing stack values are |
|
The missing PEP is easy to fix for now by generating an i32.const for it before a call. This is because we currently have an empty function pointer table, so there's nothing callable in it to invoke (let alone at the index we have on the stack.) |
Could you possibly post a disasm of what codegen looks like for this function now? |
|
I'm a bit confused about the load right before the |
EDIT: It should always be either an indirection cell's address (for a direct call) or the address of an indirection cell's address (for an indirect call of a delegate or ftn ptr) |
That makes sense, I managed to confuse myself reading the spec. |
Checkpoint Fix build Wasm doesn't have special argument registers in the traditional sense, at least not yet Checkpoint Checkpoint Checkpoint Fix build R2R completes simple call without crashing Handle different callTypes Emit indirections Add comment jit-format Fix clang build Checkpoint cleanup Cleanup Cleanup Fix erroneous ifdef Clean up putarg dependencies
…a zero since the high bits might be garbage
…working on a real impl
ab101b1 to
131a1b5
Compare
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 pull request adds initial scaffolding for call instructions and helper calls in the WebAssembly RyuJIT backend. The changes enable the compiler to generate direct calls, indirect calls, and tail calls for the WASM target.
Changes:
- Add support for call-related WebAssembly instructions (call, call_indirect, return_call, return_call_indirect)
- Implement
emitIns_Callin the WASM emitter to handle different call types - Add code generation logic for calls in
genCall,genCallInstruction, andgenEmitHelperCall - Update lowering logic to handle WASM's lack of fixed register set
- Exclude shared helper call allocation methods from WASM builds
- Enhance constant generation to handle relocations
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lower.cpp | Corrects preprocessor directive from #ifdef to #if HAS_FIXED_REGISTER_SET, adds guards for assertions that expect PutArg nodes which WASM doesn't use |
| src/coreclr/jit/instrswasm.h | Adds four new call instructions with proper opcodes and instruction formats |
| src/coreclr/jit/emitwasm.h | Removes unused method declarations for call instruction descriptors that are not needed for WASM |
| src/coreclr/jit/emitwasm.cpp | Implements emitIns_Call with support for direct and indirect calls, adds handling for IF_CALL_INDIRECT format in size calculation, output, and display methods |
| src/coreclr/jit/emitfmtswasm.h | Adds IF_CALL_INDIRECT instruction format definition |
| src/coreclr/jit/emit.cpp | Guards existing helper call allocation methods with #ifndef TARGET_WASM since WASM uses a different approach |
| src/coreclr/jit/codegenwasm.cpp | Implements call generation with genCall, genCallInstruction, and genEmitHelperCall; updates constant generation to handle relocations |
| src/coreclr/jit/codegencommon.cpp | Removes WASM-specific guard around genEmitCallWithCurrentGC, adds guard to exclude async resume functions from WASM |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I'd like to try and finalize this so we can get it in. Crossgen successfully generates a valid module, though we can't actually perform the call since stuff like relocs is missing. I think the rest of the followup work should be done in separate PRs. |
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
| } while (numMarkedNodes > 0); | ||
|
|
||
| // WASM-FIXME: !HAS_FIXED_REGISTER_SET || | ||
| assert(node->OperIsPutArg()); |
Copilot
AI
Jan 29, 2026
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.
The WASM-FIXME comment suggests that the assertion at line 3106 may fail for WASM because !HAS_FIXED_REGISTER_SET means there are no PUTARG nodes. However, this function already returns nullptr at line 3088 if numMarkedNodes is 0, which would happen when MarkPutArgAndFieldListNodes returns 0 (as it does for WASM when there are no PutArg or FieldList nodes). The assertion may still be reachable if there are FieldList nodes without PutArg nodes. Consider whether this assertion should be conditional on HAS_FIXED_REGISTER_SET, or whether the early exit logic needs adjustment.
| assert(node->OperIsPutArg()); | |
| assert(!HAS_FIXED_REGISTER_SET || node->OperIsPutArg()); |
| case IF_CALL_INDIRECT: | ||
| { | ||
| cnsval_ssize_t imm = emitGetInsSC(id); | ||
| printf(" 0 %llu", (uint64_t)imm); |
Copilot
AI
Jan 29, 2026
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.
The display format for IF_CALL_INDIRECT at line 720 shows "0 %llu" (table index, then type index), but the actual encoding at lines 539-541 outputs the type index first, then the table index. According to the WebAssembly specification, call_indirect should be encoded as: opcode, type_idx, table_idx. The encoding order is correct, but the display format should be updated to match: "printf(" %llu 0", (uint64_t)imm);" to show type index followed by table index.
| printf(" 0 %llu", (uint64_t)imm); | |
| printf(" %llu 0", (uint64_t)imm); |
|
|
||
| instruction ins; | ||
|
|
||
| // FIXME-WASM: Currently while we're loading SP onto the stack we're not loading PEP, so generate one here. |
Copilot
AI
Jan 29, 2026
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.
This hardcoded PEP (Program Entry Point) value of 0 is a temporary workaround. The comment indicates this is because SP is loaded onto the stack but not PEP. This needs to be properly implemented before this code can work correctly. Consider tracking this with a more specific TODO or issue reference.
| // FIXME-WASM: Currently while we're loading SP onto the stack we're not loading PEP, so generate one here. | |
| // TODO-WASM: Push the actual Program Entry Point (PEP) instead of the placeholder 0 value here | |
| // once PEP is properly propagated and loaded alongside SP in the WASM backend. See issue #NNNNN. |
| { | ||
| instruction ins; | ||
| cnsval_ssize_t bits; | ||
| instruction ins = INS_unreachable; |
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.
| instruction ins = INS_unreachable; | |
| instruction ins = INS_none; |
| // WASM-TODO: Generate reloc for this handle; 64-bit support | ||
| ins = INS_i32_const; |
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.
| // WASM-TODO: Generate reloc for this handle; 64-bit support | |
| ins = INS_i32_const; | |
| // WASM-TODO: Generate reloc for this handle | |
| ins = INS_I_const; |
| case TYP_DOUBLE: | ||
| } | ||
|
|
||
| if (ins == INS_unreachable) |
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.
| if (ins == INS_unreachable) | |
| if (ins == INS_none) |
|
|
||
| assert(!call->IsTailCall()); | ||
|
|
||
| genCallInstruction(call); |
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.
Arguments need to be consumed. Something like:
for (CallArg& arg : call->gtArgs.EarlyArgs())
{
genConsumeReg(arg.GetEarlyNode());
}
for (CallArg& arg : call->gtArgs.LateArgs())
{
genConsumeReg(arg.GetLateNode());
}
| // Determine return value size(s). | ||
| const ReturnTypeDesc* pRetTypeDesc = call->GetReturnTypeDesc(); |
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.
| // Determine return value size(s). | |
| const ReturnTypeDesc* pRetTypeDesc = call->GetReturnTypeDesc(); |
Unused.
| // for the purpose of GC safepointing tail-calls are not real calls | ||
| id->idSetIsNoGC(params.isJump || params.noSafePoint || emitNoGChelper(params.methHnd)); | ||
|
|
||
| #ifdef DEBUG | ||
| if (EMIT_GC_VERBOSE) | ||
| { | ||
| if (id->idIsLargeCall()) | ||
| { | ||
| printf("[%02u] Rec call GC vars = %s\n", id->idDebugOnlyInfo()->idNum, | ||
| VarSetOps::ToString(emitComp, ((instrDescCGCA*)id)->idcGCvars)); | ||
| } | ||
| } | ||
| #endif | ||
|
|
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.
| // for the purpose of GC safepointing tail-calls are not real calls | |
| id->idSetIsNoGC(params.isJump || params.noSafePoint || emitNoGChelper(params.methHnd)); | |
| #ifdef DEBUG | |
| if (EMIT_GC_VERBOSE) | |
| { | |
| if (id->idIsLargeCall()) | |
| { | |
| printf("[%02u] Rec call GC vars = %s\n", id->idDebugOnlyInfo()->idNum, | |
| VarSetOps::ToString(emitComp, ((instrDescCGCA*)id)->idcGCvars)); | |
| } | |
| } | |
| #endif |
Same here; no GCInfo tracking in emit.
| size += SizeOfULEB128(0); | ||
| size += SizeOfULEB128(emitGetInsSC(this)); |
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.
| size += SizeOfULEB128(0); | |
| size += SizeOfULEB128(emitGetInsSC(this)); | |
| size += SizeOfULEB128(emitGetInsSC(this)); | |
| size += SizeOfULEB128(0); |
Just to match the actual operand order.
| // WASM-FIXME | ||
| assert(!HAS_FIXED_REGISTER_SET || use.GetNode()->OperIsPutArg()); |
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.
| // WASM-FIXME | |
| assert(!HAS_FIXED_REGISTER_SET || use.GetNode()->OperIsPutArg()); | |
| assert(!HAS_FIXED_REGISTER_SET || use.GetNode()->OperIsPutArg()); |
This is indeed how it should be.
| } | ||
| } while (numMarkedNodes > 0); | ||
|
|
||
| // WASM-FIXME: !HAS_FIXED_REGISTER_SET || |
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.
| // WASM-FIXME: !HAS_FIXED_REGISTER_SET || |
We're still only marking putargs (PUTARG_STK, which we don't use currently), so this assert is correct as-is.
| // Indirect load of actual ftn ptr from indirection cell (on the stack) | ||
| emitIns_I(INS_i32_load, EA_PTRSIZE, 0); |
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 think we should push the concept of a function pointer being "the portable entrypoint" down into emitter. For example, we will have indirect unmanaged calls, which will use nakes table indices as function pointers. Obviously, for NAOT this won't hold as well. But it can be a TODO for now.
| // Indirect load of actual ftn ptr from indirection cell (on the stack) | |
| emitIns_I(INS_i32_load, EA_PTRSIZE, 0); | |
| // Indirect load of actual ftn ptr from indirection cell (on the stack) | |
| // TODO-WASM: temporary, move this into higher layers (lowering). | |
| emitIns_I(INS_i32_load, EA_PTRSIZE, 0); |
Depends on #123021
Starting to put together all the code we need for helper calls and calls in general.