wasmtime / PR #14066 winch: stackmaps and value refs · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14066 winch: stackmaps and value refs

Wasmtime GitHub notifications bot (Aug 02 2026 at 21:16):

macovedj opened PR #14066 from macovedj:winch-stackmaps to bytecodealliance:main:

First PR addressing https://github.com/bytecodealliance/wasmtime/issues/14057. Adds SP relative api for cranelift UserStackMap so that they can be used by winch and emitted at function calls. Also adds ref values to value stack. Bails on DRC collector, which I think can be handled in a subsequent PR adding read/write barrier support. I also enabled GC types, which I think should be fine with the bail, as it is not the default collector.

Wasmtime GitHub notifications bot (Aug 02 2026 at 21:16):

macovedj requested alexcrichton for a review on PR #14066.

Wasmtime GitHub notifications bot (Aug 02 2026 at 21:16):

macovedj requested wasmtime-compiler-reviewers for a review on PR #14066.

Wasmtime GitHub notifications bot (Aug 02 2026 at 21:16):

macovedj requested wasmtime-core-reviewers for a review on PR #14066.

Wasmtime GitHub notifications bot (Aug 02 2026 at 22:12):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

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

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

github-actions[bot] added the label cranelift:area:machinst on PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

github-actions[bot] added the label wasmtime:api on PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

github-actions[bot] added the label wasmtime:config on PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

github-actions[bot] added the label winch on PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 01:13):

github-actions[bot] commented on PR #14066:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "wasmtime:api", "wasmtime:config", "winch"

Thus the following users have been cc'd because of the following labels:

saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 03 2026 at 02:35):

github-actions[bot] commented on PR #14066:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Aug 03 2026 at 14:45):

alexcrichton commented on PR #14066:

I'm not confident enough in reviewing a mixture of Winch/stack maps. @fitzgen or maybe @saulecabrera would either of y'all be up for reviewing this?

Wasmtime GitHub notifications bot (Aug 03 2026 at 15:13):

saulecabrera commented on PR #14066:

I started taking a look.

Wasmtime GitHub notifications bot (Aug 03 2026 at 15:13):

saulecabrera edited a comment on PR #14066:

I started taking a look. I will assign it to myself.

Wasmtime GitHub notifications bot (Aug 03 2026 at 15:13):

saulecabrera requested saulecabrera for a review on PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 15:14):

saulecabrera unassigned alexcrichton from PR #14066 winch: stackmaps and value refs.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

A few thoughts on this snippet:

This is going to scan all the values in the stack per call, which I believe will affect Winch's compile time performance: I suspect the performance will decrease according to stack's length.

We are iterating all the locals here as well, which I think could be improved.

Instead, could we:

Pre-compute the local slots in the frame

Track GC objects on the value stack, through a sentinel.

Such that you can check:
if !frame.gc_ref_local_offsets.is_empty() && context.stack.gc_ref_count() != 0 { /* emit */ }
Additionally, I believe that this functionality could live on the context itself:
context.calcualte_stack_map_offsets() or similar.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:memo: saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

The name of the method here is spill_register_arguments, but here we are dealing with stack arguments, I think we either name this method differently or introduce a new function called from the prologue to handle the GC references on the stack.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

Can you add a test in tests/all/winch_engine_features.rs covering this case? Even if temporary I think it make sense to have one there until support for it is implemented.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

We have

impl TryFrom<WasmValType> for OperandSize

So perhaps here we can do

                                self.masm.store((*reg).into(), addr, (*ty).try_into()?)?;

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

I wonder if we should move this as a method under winch_codegen::stack::Val? That would also reduce how the checks are written for stack slots.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

Similar here, I think we can use try_into()

                        masm.load(src_addr, scratch.writable(), ty.try_into()?)?;
                        masm.store(scratch.inner().into(), dst_addr, ty.try_into()?)

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

The stack map emission contract:

which must be the return address of the call emitted immediately before.

Is not fully enforced: moving this statement could cause the offsets to differ. Instead, could we find a way to fold masm.emit_stack_map with masm.call? That will at least guarantee that stack map emission is tightly coupled with the call emission.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

Since we are probably moving this to the context, as suggested in https://github.com/bytecodealliance/wasmtime/pull/14066/changes#r3705202468, we should probably assert that the value here is not a register, here due to the ordering of the calls above, we assume that a spill happened, but for safety, we should probably add an ensure!(...) returning an internal error (which implies a compiler bug)

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

Trying to avoid surprises down the line (e.g., a wrap will produce a wrong offset)
                .map(|slot| sp.as_u32().checked_sub(slot.offset).expect(...)),

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:32):

:speech_balloon: saulecabrera created PR review comment:

On a second thought, I believe this will break the current state of the fuzzers. A potential alternative to consider is dynamically adding WasmFeatures::GC_TYPES to the list of unsupported features when the collector is Collector::DeferredReferenceCounting; that should leave the state of the configuration in a similar state as before.

Wasmtime GitHub notifications bot (Aug 03 2026 at 18:53):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 19:21):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 19:24):

:memo: macovedj submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2026 at 19:24):

:speech_balloon: macovedj created PR review comment:

I went ahead and made a test and dynamically added GC types

Wasmtime GitHub notifications bot (Aug 03 2026 at 19:24):

:memo: macovedj submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2026 at 19:24):

:speech_balloon: macovedj created PR review comment:

I went with splitting it out into another function called in the prologue

Wasmtime GitHub notifications bot (Aug 03 2026 at 21:01):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 03 2026 at 21:15):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 05 2026 at 13:00):

:memo: saulecabrera submitted PR review:

Looking good. Left a couple of comments that I think should be addressed before merging.

Wasmtime GitHub notifications bot (Aug 05 2026 at 13:00):

:speech_balloon: saulecabrera created PR review comment:

To avoid repeating this check in each of the backends, I believe we could fold it into Context::calculate_stack_maps?

Wasmtime GitHub notifications bot (Aug 05 2026 at 13:00):

:speech_balloon: saulecabrera created PR review comment:

Given with the recent changes this is only used in each of the Masm implementations, I wonder if we could remove it?

Alternatively though, I like the fact of having a trait method, since it forces each of the backends to provide an implementation; perhaps we could change the process a bit:

Instead of emitting the stack maps directly, we could have Masm::call receive an additional closure:
fn call(
        &mut self,
        stack_args_size: u32,
        context: &mut CodeGenContext<Emission>,
        f: impl FnMut(
            &mut Self
        ) -> Result<(CalleeKind, CallingConvention)>,
        finalize: impl FnMut(
           &mut Self,
           &mut CodeGenContext<Emission>,
        )
    ) -> Result<u32>;
Then in call.rs, we could call the stack map calculation. I think this has the added advantage that:

Removes the duplication per backend

Allows us to keep the trait method

Allows us to minimize the Masm changes to contain purely ISA specific code.

Wasmtime GitHub notifications bot (Aug 05 2026 at 13:00):

:speech_balloon: saulecabrera created PR review comment:

One small thing here, perhaps we could use the already existing SPOffset struct for type safety.

Wasmtime GitHub notifications bot (Aug 05 2026 at 13:00):

:speech_balloon: saulecabrera created PR review comment:

Could we store the offsets in Self::compute_arg_slots? That way we can avoid iterating over all the locals.

Wasmtime GitHub notifications bot (Aug 05 2026 at 15:16):

macovedj updated PR #14066.

Wasmtime GitHub notifications bot (Aug 05 2026 at 16:59):

:thumbs_up: saulecabrera submitted PR review:

LGTM. Thanks for your patience iterating on this.

Wasmtime GitHub notifications bot (Aug 05 2026 at 16:59):

saulecabrera added PR #14066 winch: stackmaps and value refs to the merge queue.

Wasmtime GitHub notifications bot (Aug 05 2026 at 17:27):

:check: saulecabrera merged PR #14066.

Wasmtime GitHub notifications bot (Aug 05 2026 at 17:27):

saulecabrera removed PR #14066 winch: stackmaps and value refs from the merge queue.

Last updated: Aug 30 2026 at 09:07 UTC