Stream: git-wasmtime

Topic: wasmtime / PR #14972 Cranelift: merge readonly (but not n...

Wasmtime GitHub notifications bot (Oct 23 2026 at 21:19):

cfallin requested alexcrichton for a review on PR #14972.

Wasmtime GitHub notifications bot (Oct 23 2026 at 21:19):

cfallin requested wasmtime-compiler-reviewers for a review on PR #14972.

Wasmtime GitHub notifications bot (Oct 23 2026 at 21:21):

cfallin opened PR #14972 from cfallin:merge-readonly-loads to bytecodealliance:main:

In the last Cranelift meeting, we discussed the semantics of load merging and the various memory flags, such as can_move, readonly, and notrap, and which of these permits GVN'ing redundant loads together.

We concluded that for a load marked only readonly (so, may trap and cannot move), it should still be safe to GVN that load against earlier identical loads that dominate it. THe current egraph mid-end does not do this.

The GVN-of-side-effecting-but-idempotent ops is fundamentally gated by is_mergeable_for_egraph, and in that function, we previously excluded all loads and stores, with the rationale that (i) readonly + can-move + notrap loads are truly pure and will be handled by the ordinary egraph GVN mechanisms, and (ii) all lother loads and stores are subject to whatever our alias analysis does.

These loads occupy a middle ground: they cannot float and rematerialize anywhere (as a pure op could), but alias analysis is also too weak to do useful things in many situations, e.g. when the GVN might occur across a callsite. We'd still like to reason about the fundamental implication of readonly on its own: once any load to the address has occurred (at an original location that hasn't been code-motioned, etc), and if we continue past that point (no trap), then readonly itself implies that the loaded value will never change -- so we can rewire subsequent identical loads to use the original value.

This makes the sequence

    v2 = load.i32 readonly v1
    call fn0()
    v3 = load.i32 readonly v1

properly GVN v3 onto v2.

Addresses the issue observed in #14683 and discussed in this Zulip thread.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.md

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 23 2026 at 21:23):

cfallin commented on PR #14972:

(On running disas tests locally I see a panic now, will revise in a bit)

Wasmtime GitHub notifications bot (Oct 23 2026 at 21:59):

cfallin updated PR #14972.

Wasmtime GitHub notifications bot (Oct 23 2026 at 22:01):

cfallin edited a comment on PR #14972:

(On running disas tests locally I see a panic now, will revise in a bit) EDIT: passes tests now

Wasmtime GitHub notifications bot (Oct 24 2026 at 01:08):

github-actions[bot] added the label cranelift on PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 14:44):

:thumbs_up: alexcrichton submitted PR review:

This all seems reasonable to me, but @fitzgen should probably also take a look since he's more familiar with the egraph bits than I

Wasmtime GitHub notifications bot (Oct 26 2026 at 14:44):

alexcrichton requested fitzgen for a review on PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 15:32):

:thumbs_up: fitzgen submitted PR review:

This LGTM, but can you do a targeted LLM audit on Wasmtime's Wasm-to-CLIF translation before landing? Just to make sure that this new behavior doesn't interact poorly with the CLIF we currently generate for interacting with Wasm state and the VM* structures and all that.

Wasmtime GitHub notifications bot (Oct 26 2026 at 15:32):

:speech_balloon: fitzgen created PR review comment:

Should this debug assert that there is an entry already in here, since IIUC we are only doing this when updating an existing GVN map entry?

Wasmtime GitHub notifications bot (Oct 26 2026 at 15:32):

:speech_balloon: fitzgen created PR review comment:

Because the branch arm that doesn't set the gvn_key returns, I think we can simplify this a little bit into:

        //
        // If we insert this instruction into the GVN map as a new
        // entry, remember its key: alias analysis below may still
        // remove it (e.g. a mergeable `readonly` load), in which case
        // the GVN entry must be updated to name the replacement value
        // rather than this instruction's (soon-removed) result.
        let gvn_key = if is_mergeable_for_egraph(self.func, inst) {

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:31):

cfallin updated PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:32):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:32):

:speech_balloon: cfallin created PR review comment:

Good call; added.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:32):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:32):

:speech_balloon: cfallin created PR review comment:

Ah, yep, that's cleaner, thanks -- changed.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:33):

cfallin commented on PR #14972:

Thanks! I did an audit with /wasmtime-auditor on the Wasm-to-CLIF translator and it didn't find any places where the usage of the readonly flag would conflict with this new optimization.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:33):

cfallin has enabled auto merge for PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 21:42):

cfallin added PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. to the merge queue

Wasmtime GitHub notifications bot (Oct 26 2026 at 22:06):

github-merge-queue[bot] removed PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. from the merge queue

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:03):

cfallin updated PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:03):

cfallin has enabled auto merge for PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:13):

cfallin added PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. to the merge queue

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:37):

github-merge-queue[bot] removed PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. from the merge queue

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:41):

alexcrichton commented on PR #14972:

good news: it works!

bad news: sorry about the merge conflict :(

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:41):

cfallin removed the review request for wasmtime-core-reviewers from PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:41):

cfallin requested dicej for a review on PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:41):

cfallin updated PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:41):

cfallin requested wasmtime-core-reviewers for a review on PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:42):

cfallin commented on PR #14972:

Neat to see exactly the loads that spawned the discussion disappear, though!

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:42):

cfallin has enabled auto merge for PR #14972.

Wasmtime GitHub notifications bot (Oct 26 2026 at 23:53):

cfallin added PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. to the merge queue

Wasmtime GitHub notifications bot (Oct 27 2026 at 00:39):

cfallin removed PR #14972 Cranelift: merge readonly (but not notrap or can-move) loads. from the merge queue

Wasmtime GitHub notifications bot (Oct 27 2026 at 00:39):

:check: cfallin merged PR #14972.

Last updated: Oct 31 2026 at 23:03 UTC