[Wasm RyuJit] local stores #122390
[Wasm RyuJit] local stores #122390
Conversation
|
@dotnet/jit-contrib PTAL |
adamperlin
left a comment
There was a problem hiding this comment.
This looks good to me!
There was a problem hiding this comment.
Pull request overview
This PR adds support for local variable stores in the WebAssembly RyuJIT backend by implementing the GT_STORE_LCL_VAR operation. It introduces new WASM instructions for storing values (i32.store, i64.store, f32.store, f64.store) and the local.set instruction for register candidates.
Key changes:
- Added WASM store instructions and
local.setinstruction definition - Implemented
ins_Storefunction for WASM to map types to appropriate store instructions - Implemented
genCodeForStoreLclVarto generate code for storing to local variables
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/coreclr/jit/instrswasm.h | Adds instruction definitions for local.set and memory store instructions (i32.store, i64.store, f32.store, f64.store) with correct WASM opcodes |
| src/coreclr/jit/instr.cpp | Implements WASM-specific ins_Store function to map variable types to appropriate store instructions |
| src/coreclr/jit/codegenwasm.cpp | Adds GT_STORE_LCL_VAR case handler and implements genCodeForStoreLclVar to emit appropriate instructions for both register candidates and stack-based variables |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
I was too hasty to approve here, apologies. I was forgetting that the emitIns_S encoding will not work for stores. As SingleAccretion pointed out, the stack must be:
<target>
<value> <- top
and the store itself (I believe) should just be encoded as a single opcode. @SingleAccretion could you potentially explain a bit more how RA will handle the placement of store arguments on the stack?
|
Guessing we'll rewrite this during RA as a different OP that will have two operands so we can get the stack ordering right. |
Correct. It'll rewrite |
|
Speaking of RA, seems like this is something we'll need fairly soonish if we want to run even simple examples. Given the "unlimited" register set of Wasm, it also seems like (parts of it) should be relatively straightforward. @SingleAccretion is this something on your todo list...? |
Yes, I am working on it. A lot of time spent on reviews though :). |
|
@adamperlin you ok with this now? You need to approve. |
adamperlin
left a comment
There was a problem hiding this comment.
This looks good to me. I just had a question about the relationship between being a register candidate and a wasm local, but I don't think that's blocking!
No description provided.