Stream: git-wasmtime

Topic: wasmtime / PR #14999 Cranelift: add a `volatile` memory f...

Wasmtime GitHub notifications bot (Oct 26 2026 at 20:39):

TastyCZ opened PR #14999 from TastyCZ:cranelift-volatile-memflag to bytecodealliance:main:

This adds the volatile MemFlag discussed in #1598: a load or store with this flag happens exactly as written. Nothing removes it, merges it with another access, or folds it into another instruction.

Today a frontend has no way to ask for that. At opt_level=speed, alias analysis removes the first of two stores to the same address, replaces the second of two loads with the first, and forwards a stored value to the following load; an unused notrap load is not emitted at any opt level. rustc_codegen_cranelift needs the flag for read_volatile/write_volatile (rust-lang/rustc_codegen_cranelift#1715).

What the flag does in each pass:

• inst_predicates: a volatile load has a side effect, so it is lowered even when unused, and it is never pure for the egraph (no GVN, LICM or rematerialization).
• alias analysis: a volatile access is never replaced by a known value, never removed as an idempotent or dead store, never the source of a known value, and never makes another store dead. It is also treated as a fence: it observes every earlier store and clears what is known about memory, so nothing is forwarded across it.
• lowering: a volatile load is never sunk into its user's memory operand (which could change its width or read it twice), on every backend.
• verifier: rejects volatile together with readonly or can_move.

The flag takes the unused bit 4 of MachMemFlags, prints and parses as volatile, and is listed with the other memory flags in cranelift/docs/ir.md. Accesses without it compile as before.

Tests: new filetests alias/volatile.clif, egraph/volatile.clif, verifier/volatile.clif, isa/x64/volatile-no-sink.clif and isa/aarch64/volatile-no-sink.clif, plus a print/parse round trip in memflags.rs. With volatile on each access, the four functions from the issue keep every load and store at opt_level=speed and none.

One thing to look at in review: the fence treatment in alias analysis is the conservative choice, since it also stops forwarding between non-volatile accesses on either side of a volatile one. I'm happy to narrow it if you'd prefer volatile accesses to only stay apart from each other.

Closes #1598

Wasmtime GitHub notifications bot (Oct 26 2026 at 20:39):

TastyCZ requested cfallin for a review on PR #14999.

Wasmtime GitHub notifications bot (Oct 26 2026 at 20:39):

TastyCZ removed the review request for wasmtime-compiler-reviewers from PR #14999.

Wasmtime GitHub notifications bot (Oct 26 2026 at 20:39):

TastyCZ requested wasmtime-compiler-reviewers for a review on PR #14999.

Wasmtime GitHub notifications bot (Oct 26 2026 at 20:46):

bjorn3 commented on PR #14999:

If we were to follow the precedent of other memory flags (opting in to optimizations rather than opting out), then volatile would be the default and we did have a novolatile flag instead. What do you think @cfallin?

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

:memo: cfallin submitted PR review:

Thanks! Some initial thoughts below.

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

:speech_balloon: cfallin created PR review comment:

I would remove the "never folded into another instruction, so its count and width stay as written": the way that we do load-folding does preserve count (we only do it for singly-used loads) and width (modulo some bugs we've had, we never extend width). We also have an analysis that ensures that the load doesn't move across any other side-effecting op.

So given that, in principle I think it should be perfectly fine to fold a volatile load as we do any other: it is semantically invisible both to the current program and to other concurrent readers/writers on the memory (and to the memory hardware itself).

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

:speech_balloon: cfallin created PR review comment:

No need for this unit test -- we get plenty of coverage from the higher levels; we don't need to add 10 lines of test (and consequent compile and CI time) for a new bit in a bitfield.

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

:speech_balloon: cfallin created PR review comment:

s/or folded...//

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

:speech_balloon: cfallin created PR review comment:

Isn't the new condition already implied by the mutual exclusivity between volatile and readonly/can_move?

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

:speech_balloon: cfallin created PR review comment:

Also: we should define here the mutual-exclusion conditions that you enforce in the verifier. I think it seems right that we should enforce !(volatile && (can_move || readonly)), i.e., the volatile flag is mutually-exclusive with both can-move and readonly.

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

:speech_balloon: cfallin created PR review comment:

I don't think we want to conflate volatile accesses with trapping: they are separate things. If we need the effects of "considering a load as possibly trapping", then we can get those effects one level up where we consider predicates.

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

:speech_balloon: cfallin created PR review comment:

As above: I don't think this condition is necessary. The comment is incorrect (folding ensures that a load is only folded once), and we maintain the same width.

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

:speech_balloon: cfallin created PR review comment:

(Given that this is the case, you can add a note below that this implies not volatile, for the reader's benefit)

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

TastyCZ updated PR #14999.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Removed, and the flag's doc now states the rule !(volatile && (can_move || readonly)). Lowering folds a volatile load like any other now.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Removed.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Removed. The no-sink filetests are gone; one x64 test now shows a volatile load folded and an unused one still emitted.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Done: is_load_with_defined_trapping is unchanged now, and has_side_effect gives a volatile load its side effect on its own.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Removed, with a note that readonly/can_move imply not volatile.

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

:memo: TastyCZ submitted PR review.

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

:speech_balloon: TastyCZ created PR review comment:

Done.

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

TastyCZ edited PR #14999:

This adds the volatile MemFlag discussed in #1598: a load or store with this flag happens exactly as written. Nothing removes it or merges it with another access.

Today a frontend has no way to ask for that. At opt_level=speed, alias analysis removes the first of two stores to the same address, replaces the second of two loads with the first, and forwards a stored value to the following load; an unused notrap load is not emitted at any opt level. rustc_codegen_cranelift needs the flag for read_volatile/write_volatile (rust-lang/rustc_codegen_cranelift#1715).

What the flag does in each pass:

• inst_predicates: a volatile load has a side effect, so it is lowered even when unused, and it is never pure for the egraph (no GVN, LICM or rematerialization).
• alias analysis: a volatile access is never replaced by a known value, never removed as an idempotent or dead store, never the source of a known value, and never makes another store dead. It is also treated as a fence: it observes every earlier store and clears what is known about memory, so nothing is forwarded across it.
• verifier: rejects volatile together with readonly or can_move; the flag's documentation states that rule.

Load folding is unchanged: a volatile load can still be folded into its user's memory operand, which keeps its count and width.

The flag takes the unused bit 4 of MachMemFlags, prints and parses as volatile, and is listed with the other memory flags in cranelift/docs/ir.md. Accesses without it compile as before.

Tests: new filetests alias/volatile.clif, egraph/volatile.clif, verifier/volatile.clif, and isa/x64/volatile.clif (a volatile load is folded like any other, and an unused one is still emitted). With volatile on each access, the four functions from the issue keep every load and store at opt_level=speed and none.

One thing to look at in review: the fence treatment in alias analysis is the conservative choice, since it also stops forwarding between non-volatile accesses on either side of a volatile one. I'm happy to narrow it if you'd prefer volatile accesses to only stay apart from each other.

Closes #1598

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

:thumbs_up: cfallin submitted PR review:

Thanks, LGTM!

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

cfallin has enabled auto merge for PR #14999.

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

bjorn3 commented on PR #14999:

@cfallin did you see https://github.com/bytecodealliance/wasmtime/pull/14999#issuecomment-6426931629?

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

cfallin added PR #14999 Cranelift: add a volatile memory flag that every pass respects to the merge queue.

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

cfallin commented on PR #14999:

@bjorn3 ah, I did, and I could have sworn I replied, but maybe the GitHub UI ate it -- in any case, I think the opposite default makes sense here: making volatile the default would silently degrade every existing producer of CLIF, and would make the IR much noisier as almost all loads and stores in almost all programs would be novolatile. It makes sense to me to have to opt into these semantics; "compiler is allowed to optimized loads/stores" is the default for most optimizing compilers.

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

:check: cfallin merged PR #14999.

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

cfallin removed PR #14999 Cranelift: add a volatile memory flag that every pass respects from the merge queue.

Wasmtime GitHub notifications bot (Oct 27 2026 at 12:26):

bjorn3 commented on PR #14999:

Most optimizing compilers make unaligned, trapping loads/stores, data races, out-of-bounds accesses and more UB too by default, yet Cranelift require an explicit opt-in for all those things. But I agree that novolatile would be fairly noisy.

Wasmtime GitHub notifications bot (Oct 27 2026 at 17:27):

fitzgen commented on PR #14999:

This PR introduced a +0.79% instructions retired regression to geomean compilation on our Sightglass benchmarks, and up to +1.44% instructions retired when compiling the hashset benchmark.

Details and reproduction instructions here: https://github.com/bytecodealliance/are-we-fast-yet/issues/12

A little analysis blames ~100% of the regression on the changes to classify in alias_analysis.rs. That is, I undid the changes in this function, and basically all of the regression went away (but this obviously defeats the purpose of the volatile flag). Then I did the same thing to the has_side_effect function in lowering and basically none of the regression went away. It seems likely we could recover much of this regression by testing for volatile loads later in the classify function, after non-loads have naturally been filtered out earlier, which doesn't require accessing the dfg.mem_flags side table.

@TastyCZ is this something you would be interested in pursuing?

Wasmtime GitHub notifications bot (Oct 27 2026 at 19:09):

TastyCZ commented on PR #14999:

Thanks for bisecting and narrowing it down! Yes, happy to take it. I moved the volatile check in classify to where it already reads the memflags of loads and stores, so other instructions don't pay for it anymore: #15029.

With callgrind on Sightglass (compilation, instructions retired, against #14999's parent), it brings the geomean from +0.94% down to +0.13%, and hashset from +1.44% down to +0.16%. Dropping the check from classify completely gets +0.05%, so what's left is small. The PR description says how I measured it.

Last updated: Oct 31 2026 at 23:03 UTC