Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
|
Not sure what i'm doing wrong here yet. |
There was a problem hiding this comment.
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 |
|
cc @adamperlin see the WasmLowering.GetSignature changes. I couldn't figure out what was wrong with the existing implementation so I rewrote it, but the rewrite isn't complete. It was generating signatures with zeroes in them and that produced invalid wasm modules. |
That's probably my fault: I revised that code to try and handle the return buffer placement. @MichalStrehovsky in the managed type system signatures is the |
If you mean the
|
The re-write looks good to me, appending to a list seems like an easier solution to ensure correctness. My only feedback would just be that we should probably add a comment (maybe in the header) explaining what the maximum number of parameters we might add when lowering the signature is (explaining the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/jit/lowerwasm.cpp:484
- The DEBUG logging should use JITDUMP/JITDUMPEXEC instead of raw printf to be consistent with existing JIT logging patterns. The suggested approach is to use
JITDUMP("node==[%06u] prev==[%06u]\n", Compiler::dspTreeID(node), Compiler::dspTreeID(prev));which eliminates the need for the#ifdef DEBUGwrapper.
JITDUMP("node==[%06u] prev==[%06u]\n", Compiler::dspTreeID(node), Compiler::dspTreeID(prev));
NYI_WASM("IR not in a stackified form");
}
// In stack order, the last operand is closest to its parent, thus put on top here.
node->VisitOperands([stack](GenTree* operand) {
stack->Push(operand);
return GenTree::VisitResult::Continue;
});
|
@dotnet/jit-contrib PTAL |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Looks good overall, just curious about the GC disabling part.
Add insns for memory copy and fill Remove NYIs so stuff can flow through Cleanup Checkpoint Fix opcodes Fix WasmLowering.GetSignature Maybe-working codegen for zeroing a struct jit-format Remove unused local Consume operands Fix erroneous fallthrough Address PR feedback Remove dead call NotImplementedException ArrayBuilder and comment Make initblk loop work
Remove else case
Co-authored-by: SingleAccretion <[email protected]>
Co-authored-by: SingleAccretion <[email protected]>
Co-authored-by: SingleAccretion <[email protected]>
Co-authored-by: SingleAccretion <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Resolving all comments to merge as-is. Issues will be fixed in follow-up PRs. |
This is sufficient to compile these three managed methods to wasm functions with native
memory.fillandmemory.copyinstructions: