Skip to content

[wasm] [JIT] Contain Array Data Offsets - #135146

Open
adamperlin wants to merge 1 commit into
dotnet:mainfrom
adamperlin:adamperlin/wasm-fold-array-offset
Open

adamperlin wants to merge 1 commit into
dotnet:mainfrom
adamperlin:adamperlin/wasm-fold-array-offset

Conversation

@adamperlin

@adamperlin adamperlin commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

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

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
@adamperlin
adamperlin requested a balanced review from Copilot October 3, 2026 00:49
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 3, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) + dataOffset for 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.

@adamperlin

Copy link
Copy Markdown
Contributor Author

cc @dotnet/wasm-contrib

@adamperlin
adamperlin requested a review from EgorBo October 9, 2026 22:46
@EgorBo

EgorBo commented Oct 10, 2026

Copy link
Copy Markdown
Member

in particular, the address must be TYP_REF or TYP_BYREF

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?

@adamperlin

adamperlin commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

@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:

// The memarg offset is added in infinite precision, while "i32.add" wraps, so the two only differ when
)
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.

@EgorBo

EgorBo commented Oct 12, 2026

Copy link
Copy Markdown
Member

@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:

// The memarg offset is added in infinite precision, while "i32.add" wraps, so the two only differ when

)
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.

@adamperlin

Copy link
Copy Markdown
Contributor Author

@EgorBo got it. Yes that's the correct, on Wasm the addressing mode will trap on overflow, but if we compute the address before the load with i32.add $base $offset; i32.load, we will have wraparound on overflow. If TYP_BYREF could overflow with +cns, then maybe the right thing to do is to limit the folding like you're suggesting. I can make that fix.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[wasm] Wasm encoding of reading from array is unnecessarily large

3 participants