Initial support for vex encoding for the new assembler. #10754

jlb6740 · 2025-05-08T23:05:18Z

No description provided.

jlb6740 · 2025-05-08T23:10:19Z

The review for this patch started here #10695. This attempts to add support for vex encoding and in the process adds the lowering of couple of initial vex instructions vaddpd and vaddps.
@rahulchaphalkar

abrown

There's some smaller review comments mixed but I wanted to highlight the VexInstruction encoding as the only major thing left to resolve; otherwise this looks good to me. I left some comments here about that encoding part but we can talk offline in more detail.

abrown · 2025-05-09T16:19:30Z

cranelift/assembler-x64/meta/src/dsl/encoding.rs

@@ -408,6 +418,23 @@ impl Prefixes {
            && self.group3.is_none()
            && self.group4.is_none()
    }
+
+    pub fn bits(&self) -> u8 {


If this is for VEX encoding maybe it should go with the VEX-related code. Plus, don't we need to check the actual values in each group?

In fact, do we even use this?

No .. will remove. I believe it remained as an artifact of a rebase when in fact it could/should have been purged at that time.

abrown · 2025-05-09T16:24:09Z

cranelift/assembler-x64/meta/src/dsl/encoding.rs

+            VexPP::None => write!(f, "None"),
+            VexPP::_66 => write!(f, "_66"),
+            VexPP::_F3 => write!(f, "_F3"),
+            VexPP::_F2 => write!(f, "_F2"),


For printing VexPP and VexMMMMM, if we print using . instead of _ then we can match the manual a bit more closely. E.g., VEX.128.0F.WIG 58 /r.

I think the underscore is needed because this naming is not just used for printing, but it also used as part of syntax for an associated item:

vex.vvvv = Some(self.xmm2.enc()); // cranelift/assembler-x64/meta/src/generate/format.rs:135 vex.prefix = LegacyPrefix::.66; // cranelift/assembler-x64/meta/src/generate/format.rs:136 vex.map = OpcodeMap::_0F; // cranelift/assembler-x64/meta/src/generate/format.rs:137

where here the LegacyPrefix::.66 is not valid syntax.

Maybe I can get around this. Will see.

abrown · 2025-05-09T16:26:58Z

cranelift/assembler-x64/meta/src/dsl/format.rs

-            rm8 | rm16 | rm32 | rm64 | xmm_m32 | xmm_m64 | xmm_m128 | m8 | m16 | m32 | m64 => true,
+            al | cl | ax | eax | rax | imm8 | imm16 | imm32 | r8 | r16 | r32 | r64 | xmm1
+            | xmm2 | xmm3 | ymm1 | ymm2 | ymm3 | zmm1 | zmm2 | zmm3 => false,
+            rm8 | rm16 | rm32 | rm64 | xmm_m32 | xmm_m64 | m8 | m16 | m32 | m64 | xmm_m128


Nit: move the xmm_* locations together?

Suggested change

rm8 | rm16 | rm32 | rm64 | xmm_m32 | xmm_m64 | m8 | m16 | m32 | m64 | xmm_m128

rm8 | rm16 | rm32 | rm64 | m8 | m16 | m32 | m64 | xmm_m32 | xmm_m64 | xmm_m128

abrown · 2025-05-09T16:27:23Z

cranelift/assembler-x64/meta/src/dsl/format.rs

+    xmm1,
+    xmm2,
+    xmm3,
+    ymm1,


I'm still not convinced we need these yet. Maybe we should wait to add YMM and ZMM when we actually need those instructions.

abrown · 2025-05-09T16:29:27Z

cranelift/assembler-x64/meta/src/generate/format.rs

@@ -32,6 +31,10 @@ impl dsl::Format {
        self.generate_immediate(f);
    }

+    pub fn generate_vex_encoding(&self, f: &mut Formatter, vex: &dsl::Vex) {
+        self.generate_vex(f, vex);


Might as well inline generate_vex here since there are no other uses.

abrown · 2025-05-09T16:44:29Z

cranelift/assembler-x64/src/vex.rs

+    /// REP/REPE, but no specific meaning here -- is just an opcode extension.
+    _F3,
+    /// Operand size override and same effect as F3.
+    _66F3,


The only possibilities for the pp field are: None, 66, F3, and F2. We can pare this down and rename it Pp or Prefix?

Yes, I believe this is copied over or merged from the codegen. I called this vexPP in the DSL. Will do the same here.

abrown · 2025-05-09T16:47:02Z

cranelift/assembler-x64/src/vex.rs

+    /// formats pack this information as bits in a prefix (e.g. VEX / EVEX).
+    pub fn bits(&self) -> u8 {
+        match self {
+            OpcodeMap::None => 0b00,


The manual says 00000 is reserved for the m-mmmm field and would result in a #UD. Maybe we should remove None since we can't use it (?).

None is used as default value when creating a VexInstruction. I could change that to be an option, but the way it is now is the exact same as the current implementation in the codegen encoding:

https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/src/isa/x64/encoding/vex.rs#L27-L41

Do you think we should change this to be an option?

abrown · 2025-05-09T16:50:24Z

cranelift/assembler-x64/src/vex.rs

+    pub w: bool,
+    pub reg: u8,
+    //pub rm: XmmMem<R::ReadXmm, R::ReadGpr>,
+    pub rm: Option<XmmMem<R::ReadXmm, R::ReadGpr>>,


I don't think this (or reg) is quite right: we won't always have a memory operand here. It would seem we need to do the REX flags encoding, extract the R, X, and B fields, and pass those around here.

There are instructions like the following that we won't be able to handle with this approach:

VMOVHLPS xmm1, xmm2, xmm3

VBLENDVPS xmm1, xmm2, xmm3/m128, xmm4

If we alter the VexInstruction thing to just emit the VEX prefixes, not the whole instruction, then I think things will work better here (no more default instructions) and will be more flexible for the future. Plus, we can reuse all of the suffix stuff that should be the same (see encode_rex_suffixes).

abrown · 2025-05-09T17:05:04Z

cranelift/codegen/meta/src/gen_asm.rs

@@ -351,7 +363,7 @@ pub fn generate_isle_inst_decls(f: &mut Formatter, inst: &Inst) {
        .format
        .operands
        .iter()
-        .filter(|o| o.mutability.is_read())
+        //.filter(|o| o.mutability.is_read())


Suggested change

//.filter(|o| o.mutability.is_read())

abrown · 2025-05-09T17:05:14Z

cranelift/codegen/src/isa/x64/inst.isle

@@ -3799,7 +3799,9 @@
 (decl x64_addps (Xmm XmmMem) Xmm)
 (rule 1 (x64_addps src1 src2)
      (if-let true (use_avx))
-      (xmm_rmir_vex (AvxOpcode.Vaddps) src1 src2))
+      ;;(xmm_rmir_vex (AvxOpcode.Vaddps) src1 src2))


Suggested change

;;(xmm_rmir_vex (AvxOpcode.Vaddps) src1 src2))

abrown · 2025-05-09T17:13:41Z

cranelift/assembler-x64/meta/src/dsl/encoding.rs

+
+impl VexLength {
+    /// Encode the `L` bit.
+    pub fn bits(&self) -> u8 {


We might not use this either in the DSL phase; isn't this only necessary when we encode in the cranelift-assembler-x64 crate?

This change adds support to the new assembler for write-only operands. This implementation appeared first in [bytecodealliance#10754] but is split out here to unblock implementation of instructions that require it: multiplication, conversions, moves, etc. This starts roughly the same as what was implemented for write-only XMMs in [bytecodealliance#10754] but includes support for write-only GPRs as well and generates the temporary registers which are needed. [bytecodealliance#10754]: bytecodealliance#10754 Co-authored-by: Johnnie Birch <[email protected]>

* asm: implement write-only operands This change adds support to the new assembler for write-only operands. This implementation appeared first in [#10754] but is split out here to unblock implementation of instructions that require it: multiplication, conversions, moves, etc. This starts roughly the same as what was implemented for write-only XMMs in [#10754] but includes support for write-only GPRs as well and generates the temporary registers which are needed. [#10754]: #10754 Co-authored-by: Johnnie Birch <[email protected]> * fix: use `to_reg()` to extract the Cranelift type from `Writable` --------- Co-authored-by: Johnnie Birch <[email protected]>

jlb6740 requested a review from a team as a code owner May 8, 2025 23:05

jlb6740 requested review from abrown and removed request for a team May 8, 2025 23:05

jlb6740 mentioned this pull request May 8, 2025

Initial support for vex encoding for the new assembler. #10695

Closed

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. labels May 9, 2025

jlb6740 force-pushed the add-initial-vex-encoding branch 2 times, most recently from 78ea103 to 29c9182 Compare May 9, 2025 16:21

Initial support for vex encoding for the new assembler.

29c9182

abrown reviewed May 9, 2025

View reviewed changes

abrown mentioned this pull request May 9, 2025

asm: implement write-only operands #10759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for vex encoding for the new assembler. #10754

Initial support for vex encoding for the new assembler. #10754

jlb6740 commented May 8, 2025

jlb6740 commented May 8, 2025

abrown left a comment

abrown May 9, 2025

abrown May 9, 2025

jlb6740 May 12, 2025

abrown May 9, 2025

jlb6740 May 9, 2025 •

edited

Loading

jlb6740 May 9, 2025

abrown May 9, 2025

abrown May 9, 2025

abrown May 9, 2025

abrown May 9, 2025

jlb6740 May 14, 2025

abrown May 9, 2025

jlb6740 May 10, 2025

abrown May 9, 2025

abrown May 9, 2025

abrown May 9, 2025

abrown May 9, 2025

	rm8 \| rm16 \| rm32 \| rm64 \| xmm_m32 \| xmm_m64 \| m8 \| m16 \| m32 \| m64 \| xmm_m128
	rm8 \| rm16 \| rm32 \| rm64 \| m8 \| m16 \| m32 \| m64 \| xmm_m32 \| xmm_m64 \| xmm_m128

Initial support for vex encoding for the new assembler. #10754

Are you sure you want to change the base?

Initial support for vex encoding for the new assembler. #10754

Conversation

jlb6740 commented May 8, 2025

jlb6740 commented May 8, 2025

abrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 May 9, 2025 •

edited

Loading