Stream: git-wasmtime

Topic: wasmtime / PR #10389 asm: allow printing of virtual regis...

Wasmtime GitHub notifications bot (Mar 13 2025 at 00:11):

abrown opened PR #10389 from abrown:assembler-print-vreg to bytecodealliance:main:

#10361 identifies a situation in which we want to print assembly instructions that have not yet been register allocated. Because of this, the register slots hold virtual registers, which the assembler does not know how to pretty-print. This change overrides AsReg::to_string in a few places so that cranelift-codegen's view of registers can be pretty-printed using the existing code for doing so.

Wasmtime GitHub notifications bot (Mar 13 2025 at 00:12):

abrown requested cfallin for a review on PR #10389.

Wasmtime GitHub notifications bot (Mar 13 2025 at 00:12):

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

Wasmtime GitHub notifications bot (Mar 13 2025 at 00:12):

abrown edited PR #10389:

#10361 identifies a situation in which we want to print assembly instructions that have not yet been register allocated. Because of this, the register slots hold virtual registers, which the assembler does not know how to pretty-print. This change overrides AsReg::to_string in a few places so that cranelift-codegen's view of registers can be pretty-printed using the existing code for doing so. Closes #10361.

Wasmtime GitHub notifications bot (Mar 13 2025 at 00:13):

abrown commented on PR #10389:

@cfallin, I was going to add some tests to keep this working correctly: can you remind me how to create a virtual Reg out of thin air?

Wasmtime GitHub notifications bot (Mar 13 2025 at 01:05):

cfallin commented on PR #10389:

Thanks for addressing this!

@cfallin, I was going to add some tests to keep this working correctly: can you remind me how to create a virtual Reg out of thin air?

Sounds good; something like VReg::new(200, RegClass::Int) should work I think?

Wasmtime GitHub notifications bot (Mar 13 2025 at 01:12):

cfallin submitted PR review:

This approach is definitely workable -- I'm fine with landing this. However I note that AsReg in the assembler crate carries a Debug constraint already, meaning that we're requiring the user of the assembler library to provide a Debug-printable type -- is there any way we could make to_string handle this case at the assembler crate level by delegating to that (e.g. format!("{:?}", self)) and differeniate between the real and virtual cases by making enc() return an Option<u8>? I also feel this is a little more "honest" in the sense that not every AsReg value will be a physical register at all times. .enc().expect("need physreg for encoding") in actual emission code seems fine in that case.

If that turns out to be too complex, no worries, this existing solution is fine too.

Wasmtime GitHub notifications bot (Mar 13 2025 at 23:01):

abrown updated PR #10389.

Wasmtime GitHub notifications bot (Mar 13 2025 at 23:36):

cfallin commented on PR #10389:

I poked at this a little myself and I think I'm actually fine with the way this works currently. Modifying AsReg::enc() to return Option<u8> is in some sense closer to reality -- the register may or may not have a physical encoding -- but almost all users use it only when it does, and only pretty-printing differs, so it's probably just as good to simply document that it can panic when the reg is virtual, and make pretty-printing work explicitly instead, as you've done here. I also played a bit with the plumbing on pretty-printing -- one could invert so that to_string() in the assembler crate calls back to the embedder (Cranelift) for vregs but again it's about the same amount of code, and this works fine. So I'll say LGTM, and we can always refactor later if needed.

Thanks for adding the test as well!

Wasmtime GitHub notifications bot (Mar 14 2025 at 15:10):

abrown merged PR #10389.

Last updated: Apr 18 2025 at 04:04 UTC