-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Wasm RyuJIT] Block stores #123738
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?
[Wasm RyuJIT] Block stores #123738
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
|
Not sure what i'm doing wrong here yet. |
src/coreclr/jit/lowerwasm.cpp
Outdated
| { | ||
| blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindLoop; | ||
| src->SetContained(); | ||
| // memory.fill |
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.
in this case it's expected to emit a loop of pointer-sized stores (with 0 value) to guarantee atomicity for gc slots (which memset does not guarantee), I am not sure it's a problem today for WASM, but it's probably better to maintain that
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 PR implements infrastructure for WebAssembly block store operations in the RyuJIT compiler. It adds support for lowering block copy and initialization operations to use WASM-specific memory.copy and memory.fill instructions.
Changes:
- Adds
LowerBlockStoreimplementation for WASM target to handle block copy and initialization operations - Introduces two new WASM instructions (
memory_copyandmemory_fill) and their associated instruction format (IF_MEMCPY) - Adds emitter support for encoding and displaying the new IF_MEMCPY instruction format
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/coreclr/jit/lowerwasm.cpp | Implements LowerBlockStore to handle GT_STORE_BLK and GT_INIT_BLK operations, with partial infrastructure for memory.copy and memory.fill |
| src/coreclr/jit/instrswasm.h | Adds memory_copy and memory_fill instruction definitions with their opcodes |
| src/coreclr/jit/emitfmtswasm.h | Defines the IF_MEMCPY instruction format for memory operations with two memory indices |
| src/coreclr/jit/emitwasm.cpp | Implements encoding, size calculation, and display logic for IF_MEMCPY instruction format |
| // TODO-1stClassStructs: for now we can't work with STORE_BLOCK source in register. | ||
| // TODO-WASM: Is this true for wasm as well? | ||
| const unsigned srcLclNum = src->AsLclVar()->GetLclNum(); | ||
| comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::BlockOp)); |
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.
Consider using DoNotEnregisterReason::StoreBlkSrc instead of DoNotEnregisterReason::BlockOp for consistency with other architectures (see lowerxarch.cpp:473). StoreBlkSrc is more specific and indicates this is specifically for STORE_BLK sources.
| comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::BlockOp)); | |
| comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::StoreBlkSrc)); |
| INST(memory_copy, "memory.copy", 0, IF_MEMCPY, 0x10FC) | ||
| INST(memory_fill, "memory.fill", 0, IF_ULEB128, 0x11FC) | ||
|
|
||
|
|
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.
Extra blank line should be removed for consistency with the rest of the file, which uses single blank lines between sections.
| else | ||
| { | ||
| // memory.fill | ||
| } |
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 code path leaves blkNode->gtBlkOpKind unset (it remains BlkOpKindInvalid). When codegen encounters this node, it will fail because there's no handling for WASM memory.fill instruction yet. Either this should set an appropriate operation kind, or GT_STORE_BLK handling should be added to codegenwasm.cpp before this PR is merged.
| else | ||
| { | ||
| assert(blkNode->OperIs(GT_STORE_BLK)); | ||
| // memory.copy | ||
| } |
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 code path leaves blkNode->gtBlkOpKind unset (it remains BlkOpKindInvalid). When codegen encounters this node, it will fail because there's no handling for WASM memory.copy instruction yet. Either this should set an appropriate operation kind, or GT_STORE_BLK handling should be added to codegenwasm.cpp before this PR is merged.
| IF_DEF(F64, IS_NONE, NONE) // <opcode> <f64 immediate (stored as 64-bit integer constant)> | ||
| IF_DEF(MEMARG, IS_NONE, NONE) // <opcode> <memarg> (<align> <offset>) | ||
| IF_DEF(LOCAL_DECL, IS_NONE, NONE) // <ULEB128 immediate> <byte> | ||
| IF_DEF(MEMCPY, IS_NONE, NONE) // <memory index> <memory index> |
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 comment should be <opcode> <memory index> <memory index> to be consistent with other instruction format comments that include the opcode in their description (e.g., ULEB128, SLEB128, MEMARG).
| IF_DEF(MEMCPY, IS_NONE, NONE) // <memory index> <memory index> | |
| IF_DEF(MEMCPY, IS_NONE, NONE) // <opcode> <memory index> <memory index> |
| case IF_MEMCPY: | ||
| { | ||
| size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this)); | ||
| size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this)); |
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.
Missing break statement causes fall-through to the default case, which will call unreached() and cause a runtime error. This will prevent the IF_MEMCPY instruction format from working correctly.
| size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this)); | |
| size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this)); | |
| break; |
| cnsval_ssize_t imm = emitGetInsSC(id); | ||
| printf(" %llu %llu", (uint64_t)imm, (uint64_t)imm); | ||
| dispJumpTargetIfAny(); | ||
| } |
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.
Missing break statement causes fall-through to the IF_LOCAL_DECL case. This will result in incorrect output when displaying IF_MEMCPY instructions, as it will also print the local declaration information.
| } | |
| } | |
| break; |
No description provided.