Stream: git-wasmtime

Topic: wasmtime / PR #10629 asm: plumb fixed registers through

Wasmtime GitHub notifications bot (Apr 21 2025 at 22:59):

abrown opened PR #10629 from abrown:asm-fixed to bytecodealliance:main:

This change introduces a new Fixed wrapper type that allows us to plumb fixed registers through the assembler. These instructions are not yet used by cranelift-codegen, but will be necessary at some point as described in [#10238]. The end result of this is that up at the cranelift-codegen level, fixed registers should look like run-of-the-mill virtual registers using the appropriate types, but down in cranelift-assembler-x64 we panic if the register allocator does not give them the correct register.

[#10238]: https://github.com/bytecodealliance/wasmtime/issues/10238

<!--
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 ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 21 2025 at 22:59):

abrown requested cfallin for a review on PR #10629.

Wasmtime GitHub notifications bot (Apr 21 2025 at 22:59):

abrown requested wasmtime-compiler-reviewers for a review on PR #10629.

Wasmtime GitHub notifications bot (Apr 21 2025 at 23:15):

abrown updated PR #10629.

Wasmtime GitHub notifications bot (Apr 21 2025 at 23:24):

cfallin submitted PR review:

Looks good, thanks! Just some nits below.

Wasmtime GitHub notifications bot (Apr 21 2025 at 23:24):

cfallin created PR review comment:

Is there a reason (e.g. generic interface somewhere else) that we need to accept reg here as an argument, rather than just using the generic parameter E -- since there is only one valid value to pass in?

Wasmtime GitHub notifications bot (Apr 21 2025 at 23:24):

cfallin created PR review comment:

Nice! This is presumably because a reg that we carried as a (always the same value) u32 previously now becomes a type parameter? Good to see reductions in any case.

Wasmtime GitHub notifications bot (Apr 21 2025 at 23:24):

cfallin created PR review comment:

Ah, I see below, this enables the idiom Fixed(rax) for example, where type inference carries reg through to the generic parameter.

Maybe there could be two entry points, new() and from_const_reg(reg: u8) or similar? Maybe not a huge deal; just trying to reduce confusing APIs.

Wasmtime GitHub notifications bot (Apr 22 2025 at 00:18):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 22 2025 at 00:19):

abrown created PR review comment:

I don't think we need a different entry point and, in fact, I've been trying to remove this AsReg::new somehow. It's only during fuzzing that we need the ability to construct an AsReg. I just can't seem to pull the right Jenga blocks out to get rid of this without things falling down.

Wasmtime GitHub notifications bot (Apr 22 2025 at 00:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 22 2025 at 00:24):

cfallin created PR review comment:

Ah, I see -- a few thoughts then:

• At the very least, pub(crate) fn new(...) so it's only exposed to where fuzzing needs it (the Arbitrary impl);
• Perhaps removing the field (so it's a zero-sized struct, carrying E only at the type level); you still have E for enc() to work; the only part I'm not as clear on is where AsRef comes in and why you need a reference (vs. constructing an R on-the-fly as needed, as e.g. fn to_reg(&self) -> R { R::new(E) } or something like that)

Wasmtime GitHub notifications bot (Apr 22 2025 at 00:47):

abrown merged PR #10629.

Wasmtime GitHub notifications bot (Apr 22 2025 at 01:14):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 22 2025 at 01:14):

abrown created PR review comment:

• At the very least, pub(crate) fn new(...) so it's only exposed to where fuzzing needs it (the Arbitrary impl);

What I meant is that I want to remove the trait function AsReg::new--we can't make trait functions pub(crate), right?

• Perhaps removing the field (so it's a zero-sized struct, carrying E only at the type level); you still have E for enc() to work; the only part I'm not as clear on is where AsRef comes in and why you need a reference (vs. constructing an R on-the-fly as needed, as e.g. fn to_reg(&self) -> R { R::new(E) } or something like that)

I believe we still need the field because we need a place for the virtual register to live until register allocation happens.

Wasmtime GitHub notifications bot (Apr 22 2025 at 02:54):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 22 2025 at 02:54):

cfallin created PR review comment:

Oh, d'oh, I had missed that new was part of the trait definition too, sorry! And you're of course right that we need an actual vreg too.

Last updated: Jan 09 2026 at 13:15 UTC