Stream: git-wasmtime

Topic: wasmtime / PR #10775 x64: refactor the representation of ...

Wasmtime GitHub notifications bot (May 14 2025 at 19:33):

abrown opened PR #10775 from abrown:refactor-reg-mem-imm to bytecodealliance:main:

While reflecting on why #10762 is hard, it seemed that the underlying problem to many ISLE matching issues is our inability to actually inspect the variants of the register specific versions of RegMem and RegMemImm. We had been wrapping instances of these Reg* types, but this change pushes the variants down to: GprMem, GprMemImm, XmmMem, XmmMemAligned, XmmMemImm, and XmmMemAlignedImm. With this, it should be possible to match directly on the variants in ISLE. This does not change any functionality.

<!--
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 (May 14 2025 at 19:33):

abrown requested fitzgen for a review on PR #10775.

Wasmtime GitHub notifications bot (May 14 2025 at 19:33):

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

Wasmtime GitHub notifications bot (May 14 2025 at 19:34):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 14 2025 at 19:34):

abrown created PR review comment:

Is it possible to create a GprMem.Reg in ISLE using the wrong kind of register? If so, we could alter this type to Gpr?

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

abrown edited PR review comment.

Wasmtime GitHub notifications bot (May 14 2025 at 22:10):

abrown commented on PR #10775:

I will say, having tried to use this in #10762, that it doesn't just "solve everything." So we should judge this on whether it will make future matching easier. I know that I've reached for this kind of matching in the past but was frustrated it didn't exist.

Wasmtime GitHub notifications bot (May 18 2025 at 16:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 18 2025 at 16:34):

alexcrichton created PR review comment:

Would it be possible to type this as (Gpr (reg Gpr))? That more closely matches the semantics of the GprMem constructor in Rust

Wasmtime GitHub notifications bot (May 28 2025 at 16:13):

fitzgen created PR review comment:

Following up from today's Cranelift meeting, where we briefly discussed this PR: I want to +1 Alex's comment here.

We want to maintain the invariant that all Gpr* newtypes can only ever contain GPR registers when they aren't a memory or immediate or whatever. If we expose raw Regs in these enum variants, then users can accidentally (say transitively through a series of abstractions) do something like

if let GprMem::Reg { reg } = &mut gpr_mem {
   std::mem::swap(reg, my_xmm_reg);
}

which would break that invariant.

In order to both allow matching on GprMem et al and preserve that invariant, we need to define new enums (to allow matching) but keep the register variants as Gprs (to preserve encapsulation and the GPR invariant). This would look like the ISLE equivalent of something like this:

enum GprMem {
    Gpr { reg: Gpr },
    Mem { addr: SyntheticAmode },
}

Which is, I believe, what Alex is suggesting just above this comment.

Wasmtime GitHub notifications bot (May 28 2025 at 16:13):

fitzgen submitted PR review.

Last updated: Dec 06 2025 at 06:05 UTC