Stream: git-wasmtime

Topic: wasmtime / issue #10238 x64 New Assembler Ideas/Pain points

Wasmtime GitHub notifications bot (Feb 17 2025 at 12:01):

alexcrichton opened issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [ ] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").
• [ ] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose.
• [ ] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult.
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.

cc @abrown

Wasmtime GitHub notifications bot (Feb 17 2025 at 12:02):

alexcrichton edited issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [ ] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").
• [ ] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose.
• [ ] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult.
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.
• [ ] ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type)
• [ ] Previous ISLE constructors/emit.rs should be removed instead of acting as a catch-all (to help catch mistakes and additionally gradually clean things up)

cc @abrown

Wasmtime GitHub notifications bot (Feb 17 2025 at 12:08):

alexcrichton edited issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [ ] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").
• [ ] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose.
• [ ] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult.
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.
• [ ] ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type)
• [ ] Previous ISLE constructors/emit.rs should be removed instead of acting as a catch-all (to help catch mistakes and additionally gradually clean things up)
• [ ] If Assembler*Write* is removed from ISLE bindings (above bullets) then auto-conversions and various methods to/from other ISLE types should be removed as they shouldn't be necessary.

cc @abrown

Wasmtime GitHub notifications bot (Feb 17 2025 at 12:10):

alexcrichton commented on issue #10238:

I've got some commits addressing a few things here over at https://github.com/alexcrichton/wasmtime/commits/x64-cleanups but I'm running out of steam given how many are accumulating so I'm going to set it aside for now.

Wasmtime GitHub notifications bot (Feb 19 2025 at 19:43):

abrown commented on issue #10238:

ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type)

Makes sense; should we just merge https://github.com/alexcrichton/wasmtime/commit/89e0fb48b6cb03e4ba11b058f6209695915f9250?

The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").

Hm... while I think the ISLE integration certainly can be improved, I thought we would want to maintain a clear separation of the "read" and "read-write" types all the way up into ISLE. This allows us to distinguish when we can just pass a cranelift-codegen Gpr and when we must pass the PairedGpr thing (WritableGpr, Gpr). No?

the latter would return SideEffectNoResult ... should return ValueRegs instead of a single GPR

I was looking at this last week: I thought, "why _must_ we always emit in these ISLE constructors?" If instead we just returned the MInst::External and then had some other way to do the emit part, we could more easily fit in with all these SideEffect*, ProducesFlags*, and ConsumesFlags* structures. So, e.g., to lower instructions directly we would write something like (rule ... (emit (x64_* ...))) but would then need to teach emit to do more. Perhaps emit could call an asm::Inst::emit generated function that knows which of the operands to return?

Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.

Yeah, I stopped spending much time on fixed registers when I saw we didn't use them much in the lowerings I was looking at. I think for mul, e.g., we not only have registers that are "fixed" but also "implicit," in that they shouldn't appear in disassembly (or at least that's how I'm interpreting https://github.com/alexcrichton/wasmtime/commit/8344547ec80f79b3a5bf201611f734628ad1b83e). I'm not sure I like extending the Regs trait with all kinds of specific registers; why can't we construct a specific register on one side (e.g., assembler) and use its HW encoding to map to the right thing on the other (e.g., cranelift-codegen)? Do we need additional information re: register class?

Wasmtime GitHub notifications bot (Feb 19 2025 at 19:51):

abrown commented on issue #10238:

(apologies for not being at the Cranelift meeting — other meetings interfered but I would have preferred to talk about this in person!)

Wasmtime GitHub notifications bot (Feb 19 2025 at 20:51):

alexcrichton commented on issue #10238:

Makes sense; should we just merge https://github.com/alexcrichton/wasmtime/commit/89e0fb48b6cb03e4ba11b058f6209695915f9250?

Sure!

I thought we would want to maintain a clear separation of the "read" and "read-write" types all the way up into ISLE.

Currently though ISLE is almost entirely in "SSA form" where the only use of WritableReg is in helpers which insert a WritableReg in to the MInst variants. Almost all instruction signatures don't deal with WritableReg.

In that sense I was thinking that the operands to an instruction should never be writable since everything is modeled as SSA, meaning that by default once something is produce you should lose the writable bit anyway. That means that all operands are AssemblerReadGpr{,Mem}. Additionally results are always AssemblerReadGpr since after it's produced it's not valid to overwrite a value in a register. (this also connects to AssemblerReadGprMem doesn't make sense to me as a result because you can't produce a result that resides in memory, hence my thinking of two ISLE constructors here)

To be clear though I'm not saying we should change the representation of the instructions. What I'm thinking is we change the signatures of the ISLE constructors and the implementation of the constructors (e.g. the generated macro) but that's it. The instructions continue to hold PairedGpr as they do today which matches what MInst is already doing.

I was looking at this last week: I thought, "why must we always emit in these ISLE constructors?"

I agree with your points! I was looking at adc briefly and integrating it with the external assembler seemed tricky because it was going to be hard to get the MInst.

To me the hard part here is that ISLE doesn't easily support returning pairs of values. Constructors in theory need to return both the result (e.g. AssemblerReadGpr) as well as the instruction (MInst::External). Right now they only return the AssemblerReadGpr and eject the MInst out-the-side. My best thinking for handling this is that we'd probably want flags on the inst constructor in the DSL to say something like "this consumes flags" or "this both consumes and produces flags" and that generates various variants of ISLE constructors automatically. More-or-less side-stepping the problem of working with MInst by generating more constructors.

I'm not sure I like extending the Regs trait with all kinds of specific registers; why can't we construct a specific register on one side (e.g., assembler) and use its HW encoding to map to the right thing on the other (e.g., cranelift-codegen)?

I'm not sure I quite understand what you're thinking here, could you elaborate?

What I'm thinking is that on the cranelift-codegen side we fundamentally have a vreg rather than an actual register. During register allocation that'll get fixed to a particular register but the representation of the instruction has to have a slot for Gpr somewhere inside of it (e.g. mul would somehow contain two WritableGpr and two Gpr values inside of it). My thinking was that the existing ReadGpr and ReadWriteGpr could be used (we'd regardless have to add a WriteGpr) it hinders use cases such as fuzzing where mul is only valid of the results are rax:rdx and plumbing that through to fuzzing to only generate those registers seemed easiest by having a new associated type. I do agree it's not pretty though.

Wasmtime GitHub notifications bot (Feb 22 2025 at 05:45):

alexcrichton edited issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [ ] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").
• [ ] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose.
• [ ] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult.
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.
• [x] ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type)
• [ ] Previous ISLE constructors/emit.rs should be removed instead of acting as a catch-all (to help catch mistakes and additionally gradually clean things up)
• [ ] If Assembler*Write* is removed from ISLE bindings (above bullets) then auto-conversions and various methods to/from other ISLE types should be removed as they shouldn't be necessary.

cc @abrown

Wasmtime GitHub notifications bot (Feb 22 2025 at 05:45):

alexcrichton edited issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [ ] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write").
• [ ] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose.
• [ ] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult.
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.
• [x] ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type) (#10260)
• [x] Previous ISLE constructors/emit.rs should be removed instead of acting as a catch-all (to help catch mistakes and additionally gradually clean things up) (#10260)
• [ ] If Assembler*Write* is removed from ISLE bindings (above bullets) then auto-conversions and various methods to/from other ISLE types should be removed as they shouldn't be necessary.

cc @abrown

Wasmtime GitHub notifications bot (Feb 23 2025 at 18:23):

alexcrichton edited issue #10238:

I've been poking at the new x64 assembler and I'm starting to accumulate a number of various things here and there so I wanted to write these down before I forgot. I had a rough high-level goal of "I'd like to implement mul (and imul) instructions" and it led to most of these issues:

• [ ] Fixed registers don't show up in ISLE right now. We need to represent them as Gpr in Cranelift but something like Rax during fuzzing.
• [x] The first argument of many ISLE constructors is AssemblerReadWriteGpr but this technically isn't correct, they should all take AssemblerReadGpr (note the lack of "Write"). (#10276)
• [x] Return values from ISLE are AssemblerReadWriteGpr, but like above they should probably be AssemblerReadGpr instead. Basically the "write" part should mostly just be an internal detail that the ISLE bindings don't expose. (#10276)
• [x] Instructions that modify memory, such as x64_addb_mi, should probably have two constructors in ISLE: one for the version that modifies a register and one that modifies memory. The former would return AssemblerReadGpr and the latter would return SideEffectNoResult. (#10276)
• [ ] Instructions that return multiple results, such as mul, should return ValueRegs instead of a single GPR.
• [x] ISLE constructors for new instructions can overlap in priorities when the types are disjoint (e.g. no need to do 1-12, only 1-3 for within one type) (#10260)
• [x] Previous ISLE constructors/emit.rs should be removed instead of acting as a catch-all (to help catch mistakes and additionally gradually clean things up) (#10260)
• [x] If Assembler*Write* is removed from ISLE bindings (above bullets) then auto-conversions and various methods to/from other ISLE types should be removed as they shouldn't be necessary. (#10276)

cc @abrown

Last updated: Feb 28 2025 at 02:27 UTC