Repository navigation
[wasm] [JIT] Contain Array Data Offsets - #135146
adamperlin wants to merge 1 commit into
Conversation
Group managed array bases with scaled indices so existing Wasm address containment can fold the constant data offset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b811db0-9eed-49dc-b08d-e23832b0da50
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The JIT transformation appears sound, but its claimed Wasm code-size improvement lacks direct validation in the PR.
Review effort: Balanced
Findings: None
What changed in this PR
Optimizes Wasm array access code generation by reshaping address arithmetic so the array data offset can fold into the memory instruction.
Changes:
- Uses
(array + scaledIndex) + dataOffsetfor eligible non-struct arrays. - Restricts reshaping to GC-typed array references.
| File | Description |
|---|---|
src/coreclr/jit/morph.cpp |
Adds Wasm-specific array address grouping for memarg containment. |
|
cc @dotnet/wasm-contrib |
I am not sure I understand why we check this. I assume arr_ref can also be a TYP_I_IMPL (e.g. local alloc) but is there a reason to limit on that? |
|
@EgorBo the only reason to limit is because the offset folding logic we have in place only supports TYP_REF/TYP_BYREF (See the comment here: runtime/src/coreclr/jit/lowerwasm.cpp Line 174 in 523a36f The issue is with different overflow behavior between a non-contained and contained address computation on Wasm. So, I think we could allow TYP_I_IMPL here, it just wouldn't benefit from the folding based on the rules we have in place. |
@adamperlin my understanding that on wasm the addressing mode has to never overflow and we can rely on live GC pointers + offset to never overflow - which is legal. But TYP_BYREF itself has no guarantee - it can be just some arbitrary native address and +cns may overflow it. So technically only TYP_REF is legal. But I think in this case it's fine, the arrayObj is either stack allocated or real gc ref. |
|
@EgorBo got it. Yes that's the correct, on Wasm the addressing mode will |
Resolves #134465; We can re-order an array index computation to allow the array data offset to fold into a load/store using our existing containment logic, subject to its constraints (in particular, the address must be TYP_REF or TYP_BYREF). It turns out that this kind of reordering is actually already done for risc v.
Diffs