Stream: git-wasmtime

Topic: wasmtime / PR #10687 x64: remove `Inst::AluRmiR`

Wasmtime GitHub notifications bot (Apr 28 2025 at 18:32):

abrown opened PR #10687 from abrown:remove-alu-again to bytecodealliance:main:

This change completely removes the Inst::AluRmiR variant along with associated code. It updates ABI code to use new Inst helpers and switches Winch to use the new assembler directly. More details in each commit; note that this significantly changes the disassembly tests.

Wasmtime GitHub notifications bot (Apr 28 2025 at 18:48):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2025 at 18:48):

abrown created PR review comment:

@saulecabrera, this line is causing issues like the following:

error[E0432]: unresolved import `cranelift_assembler_x64`
  --> winch/codegen/src/isa/x64/asm.rs:35:5
   |
35 | use cranelift_assembler_x64 as asm;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no external crate `cranelift_assembler_x64`

For more information about this error, try `rustc --explain E0432`.

I believe this is because the cranelift-assembler-x64 dependency is optional and setting a feature in build.rs is too late for Cargo. What do you want to do? One option is to depend on cranelift-assembler-x64 unconditionally. (I'm also interested in your comments on the rest of these Winch changes...)

Wasmtime GitHub notifications bot (Apr 28 2025 at 18:49):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Apr 30 2025 at 15:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 15:43):

alexcrichton created PR review comment:

Briefly discussed at the Cranelift meeting just now, but wanted to write it down here too -- the way we detect backends-to-enable as features in Cranelift (and by extension Winch) is not compatible with Cargo optional dependencies. This means that we can't actually have true optional dependencies per-backend, so the only solution here is to unconditionally depend on cranelift-assembler-x64. That being said it's a pretty small crate that's not too hard to build so I also don't think that's an issue.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:40):

abrown updated PR #10687.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:41):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:41):

abrown created PR review comment:

Ok, I edited the commits based on this plus some tweaks to make some immediates smaller.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

abrown has marked PR #10687 as ready for review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

abrown requested fitzgen for a review on PR #10687.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

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

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

abrown requested wasmtime-core-reviewers for a review on PR #10687.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

abrown requested wasmtime-default-reviewers for a review on PR #10687.

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

abrown submitted PR review.

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

abrown created PR review comment:

There are other ways of doing this (e.g., using the Inst::<alu> helpers) but this is what I landed on given some comments I heard that Winch should just emit assembler instructions directly. The disadvantage is that, for these immediate encodings, we could be using the "sign-extend when it fits in 8 bits" forms for smaller code size but I just chose not to. Some additional logic could be added here for that, though...

Wasmtime GitHub notifications bot (Apr 30 2025 at 19:17):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 19:17):

alexcrichton created PR review comment:

This "TODO" should be ok because this is all post-regalloc so no need to worry about uninit/etc

Wasmtime GitHub notifications bot (Apr 30 2025 at 19:17):

alexcrichton created PR review comment:

Would it be possible to replace the RegMemImm argument with just a Reg? And if there's only a small handful of locations using an Imm switching them to using the external assembler directly?

Wasmtime GitHub notifications bot (Apr 30 2025 at 19:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 19:18):

alexcrichton created PR review comment:

Or, alternatively, could these helpers be purged entirely in favor of using the external assembler directly?

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera created PR review comment:

I know that this this is pre-existing, but I've been meaning to "fix" the order of the parameters to ensure that they are consistent and more importantly that they match the ISA level definition. Not something that I'm expecting you to take on though, but it makes it clear that it's a needed changes in light of the introduction of the assembler.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera created PR review comment:

There are other ways of doing this (e.g., using the Inst::<alu> helpers) but this is what I landed on given some comments I heard that Winch should just emit assembler instructions directly.

Sounds reasonable, however, I'm still getting caught up with the structure of the new x64 assembler, could you point me to where are those helpers defined? (couldn't locate them with a quick grep). My main question here would be: are those helpers defined in the new assembler and are they doing a similar lowering like the one in here?

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera created PR review comment:

Catching up here. Upon a first look, I think it makes sense to depend on this crate unconditionally, for simplicity. We can always revisit later if needed.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera created PR review comment:

Oh, taking a closer look those helpers seem to be the ones that are lowered through lower_alu!? If that's the case, I sense a bit of repetition, but I think it's fine to diverge and keep things the way you have them in this PR; that gives Winch the freedom to evolve the lowering rules more independently, plus I think there's probably going to be less indirection this way.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:28):

saulecabrera created PR review comment:

The disadvantage is that, for these immediate encodings, we could be using the "sign-extend when it fits in 8 bits" forms for smaller code size but I just chose not to

I think it's fine not to introduce this, if I remember correctly, we didn't have this optimization before anyway. Perhaps we could consider adding a TODO at the top of the module to track these potential optimizations?

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:30):

abrown updated PR #10687.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:44):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:44):

abrown created PR review comment:

Ok, b197c14 does just that, but I can't claim things are very pretty. There's an ergonomics issue here that can absolutely be solved later, but I would be interested in discussing this at some point. Here are some thoughts on making this _look_ nicer:

• to avoid repetitive .into() conversions for each operand, perhaps we could make the assembler constructor functions take parameters of impl Into<...>?
• to avoid repetitive .into() conversions to create Inst::Externals, perhaps we could create an Inst::external(...) helper to do some of this more succinctly?
• to avoid repetitive checks for immediate sizes (i.e., "does this fit in an 8-bit immediate? no? ok, let's use a 32-bit...") we might be able to reuse the ABIMachineSpec::gen_add_imm helper? If that were to take an i32 instead of a u32, we could probably replace all the ~15 sub and add instances everywhere with by flipping the sign for sub
• to avoid asm:: everywhere... well, what is the conventional thing to do about that?
• to avoid wrapping immediates in asm::Simm* and asm::Imm*, we could refactor things in the assembler to just take the raw Rust types; right now the wrapper is used for pretty-printing and details like that but there's surely another way to do that.

Thoughts?

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:51):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:51):

abrown created PR review comment:

I'm still getting caught up with the structure of the new x64 assembler

Maybe I should improve the documentation! The lower_alu! macro is now gone and it was just a side helper anyways. These new functions are all being generated from instruction definitions like those in this file:

https://github.com/bytecodealliance/wasmtime/blob/28e26fdf7e389148c5725a58f0627693c525b170/cranelift/assembler-x64/meta/src/instructions/and.rs#L27-L30

The cranelift-assembler-x64 exposes all that generated code. If you want to see it, run cd cranelift/assembler-x64; cargo run and open the file that gets spit out of that.

I sense a bit of repetition, but I think it's fine to diverge and keep things the way you have them in this PR; that gives Winch the freedom to evolve the lowering rules more independently, plus I think there's probably going to be less indirection this way.

Yeah, that was what I was concerned about as well. But, yeah, "more independent" and "more direct" are the benefits here. I am still thinking of through the best way to do this so if you have any recommendations on making this more ergonomic and less repetitive, I'm interested!

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 23:52):

abrown created PR review comment:

Yeah, good point; I almost switched everything around here but then thought, "let's keep this small if we can."

Wasmtime GitHub notifications bot (May 01 2025 at 14:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 14:35):

alexcrichton created PR review comment:

I like the idea of using impl Into<...> in the ctor arguments to assist with host-level ergonomics, it's pretty similar to how things work with (convert ...) in ISLE. I could go either way on Inst::external, and another way to shorten things would be to use inst::foo instead of asm::inst::foo? Or perhaps trait EmitExt { .. } just to give an emit method?

For immediates I still think that one option we could do is, just before emission, to "compress" the immediate. For example here we could match on a arithmetic instructions like add/sub which use the 32-bit immediate form and optionally compress them to the 8-bit form. I don't think that would have any meaningful impact on compile time, wouldn't affect any semantics, and would avoid the need to do this everywhere. Alternatively having Inst::addq_mi as a helper I also think would be reasonable where it's still a wrapper around the assembler but an even thinner wrapper around the assembler than the one that previously took RegMemImm and only does immediate-checking.

I also think it'd be reasonable to refactor the assembler to use raw Rust types instead of asm::{Si,I}mm types too to assist with verbosity.

Near-term what about the impl Into<...> plus op-with-immediate helpers on Inst? Longer-term could consider something like the mutate-the-instruction-just-before-emit or something like that.

Wasmtime GitHub notifications bot (May 02 2025 at 03:40):

abrown updated PR #10687.

Wasmtime GitHub notifications bot (May 02 2025 at 03:41):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 02 2025 at 03:41):

abrown created PR review comment:

Ok, latest two commits handle any near-term case.

Wasmtime GitHub notifications bot (May 02 2025 at 03:41):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (May 02 2025 at 14:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 02 2025 at 15:04):

alexcrichton merged PR #10687.

Last updated: Dec 06 2025 at 06:05 UTC