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

jlb6740 · 2025-04-29T01:34:32Z

Patch is first, one or two more will be pushed for enabling.

alexcrichton · 2025-04-30T18:45:19Z

cranelift/assembler-x64/src/vex.rs

+use crate::api::{AsReg, CodeSink, KnownOffsetTable, Registers};
+use crate::mem::emit_modrm_sib_disp;
+use crate::Amode;
+use cranelift_assembler_x64_meta::dsl::{OpcodeMap, Vex};


Would it be possible to duplicate necessary types here? The intent is that the meta crate is only used at build-time and isn't a runtime dependency but this is otherwise adding the meta crate as a runtime dependency as well

In the cranelift-codegen cinematic universe of crates, we have -meta and -shared; the -shared crate is depended on by both the meta and main crate. Perhaps the same split makes sense here?

Hi @alexcrichton, I think so. I did it this way as it seemed to be required for a previous iteration of this patch set, plus it assures consistency and avoids duplication, but I understand we don't want a dependency on the meta crate and that duplication is ok. If I do believe there is a dependency we want to keep, I'll follow @cfallin advise and see if a shared crate makes sense here. Also note, there are a few unnecessary elements in the vex struct in the meta crate that I am removing along with commented out code intended to help develop the follow-up patch 2 of 2 to be submitted later when we enable this.

@alexcrichton .. I believe this is addressed now.

Also removes unneeded code and code cleans-up extraneous comments.

abrown

I got about halfway through reviewing this; here are some comments.

abrown · 2025-05-01T00:59:21Z

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

+    pub w: bool,
+    pub r: bool,
+    pub wig: bool,
+    pub rxb: u8,


I think at the DSL level we may want to have separate booleans for each bit, e.g.:

r: bool, x: bool, b: bool, w: bool,

As it stands, this duplicates the r bit.

Yes, I am not positive, but I don't even think any of these bits will need to be set at the DSL level. For now I'll just remove the rxb and add the b and w bits as suggested here (but maybe we can remove later).

abrown · 2025-05-01T01:01:44Z

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

+        Self { mmmmm, ..self }
+    }
+
+    fn validate(&self, _operands: &[Operand]) {}


It will be helpful to check here for invalid instruction definitions: that way we can find them early, at codegen time.

Yes agreed .. can we implement this validate in the next PR where we turn the plumbing on and we can see that the validation is actually doing the right thing against those first few instructions or would you like to see a little more of this filled out now?

abrown · 2025-05-01T01:03:15Z

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

-    pub fn bytes(&self) -> u8 {
-        self.bits() / 8
+    pub fn bytes(&self) -> u16 {
+        self.bits() / 16


This is not right:

Suggested change

self.bits() / 16

self.bits() / 8

abrown · 2025-05-01T01:04:55Z

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

+    zmm2,
+    zmm3,
+
+    xmm_m128,


This is the same as rm128... I'm not convinced adding these unused variants is necessary at this point, either.

I was wanting to be consistent with the manual here:

ADDPD xmm1, xmm2/m128
VADDPD xmm1, xmm2, xmm3/m128

and writing something like this:

inst( "vaddpd", fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]), vex(0x58).length(_128).pp(_66).mmmmm(_OF), _64b | compat | sse, ),

instead of like this:

inst( "vaddpd", fmt("B", [w(xmm1), r(xmm2), r(rm128)]), vex(0x58).length(_128).pp(_66).mmmmm(_OF), _64b | compat | sse, ),

I kept the rm128 because I wasn't sure of opinions of this during review that I replaced rm128 with xmm_m128 in all the rex encoded instructions just yet (maybe a separate patch), but using the naming xmm_m128 is more consistent with the manual. What do you think?

abrown · 2025-05-01T01:08:16Z

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

+        fmtln!(f, "match &self.xmm_m128 {{");
+        fmtln!(f, "XmmMem::Xmm(r) => {{vex.rm =  XmmMem::Xmm(r.clone());}}");
+        fmtln!(f, "XmmMem::Mem(m) => {{vex.rm =  XmmMem::Mem(m.clone());}}");
+        fmtln!(f, "}}");


When you're adding brace-delimited blocks, you can use Formatter::add_block to get the indentation right, avoid forgetting to add a closing brace, and to match the rest of this crate:

Suggested change

fmtln!(f, "match &self.xmm_m128 {{");

fmtln!(f, "XmmMem::Xmm(r) => {{vex.rm = XmmMem::Xmm(r.clone());}}");

fmtln!(f, "XmmMem::Mem(m) => {{vex.rm = XmmMem::Mem(m.clone());}}");

fmtln!(f, "}}");

f.add_block("match &self.xmm_m128", |f| {

fmtln!(f, "XmmMem::Xmm(r) => {{vex.rm = XmmMem::Xmm(r.clone());}}");

fmtln!(f, "XmmMem::Mem(m) => {{vex.rm = XmmMem::Mem(m.clone());}}");

});

This seems a bit redundant to match here in any case...

abrown · 2025-05-01T01:11:51Z

cranelift/assembler-x64/meta/src/instructions/addpd.rs

+
+pub fn list() -> Vec<Inst> {
+    vec![
+        inst("addpd", fmt("A", [rw(xmm1), r(align(rm128))]), rex([0x66, 0x0F, 0x58]).r(), _64b | compat | sse),


This duplicates one of the definitions in add.rs, right? Let's just keep all the addition in that file to avoid this kind of mistake.

For the final vex support I removed the one in add.rs (but put it back for this pr). The reason is simple, I was organizing the instructions the same way they are organized in the manual by opcode (specifically Vol 2A 3-34 for this instruction). The ADDPD opcode instruction section covers rex, vex, and evex. What do you think about this organization of grouping instructions consistent with the manual?

abrown · 2025-05-01T01:12:29Z

cranelift/assembler-x64/src/fuzz.rs

@@ -274,6 +277,6 @@ mod test {
        .budget_ms(1_000);

        // This will run the `roundtrip` fuzzer for one second. To repeatably
-        // test a single input, append `.seed(0x<failing seed>)`.
+        // test a single0 input, append `.seed(0x<failing seed>)`.


Suggested change

// test a single0 input, append `.seed(0x<failing seed>)`.

// test a single input, append `.seed(0x<failing seed>)`.

abrown · 2025-05-01T01:15:06Z

cranelift/assembler-x64/src/vex.rs

+            imm: None,
+        }
+    }
+}


We shouldn't need to create a default (i.e., invalid) VEX instruction ever, right?

I don't think so. Will remove this.

abrown · 2025-05-01T01:16:20Z

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

+    fn generate_vex(&self, f: &mut Formatter, vex: &dsl::Vex) {
+        f.empty_line();
+        f.comment("Emit VEX prefix.");
+        fmtln!(f, "let vex: VexInstruction<R> = VexInstruction::default();");


Instead of creating a default (i.e., invalid) instruction here, why not use a new constructor or a builder pattern (e.g., op(...).reg(...)...) to create only valid instructions?

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

Hi @cfallin @alexcrichton @abrown I've attempted to address all comments or at least touch base with the reviewer offline. I've moved this PR to #10754 where instead of doing this in 2 parts, well just attempt to switch on the lowering of vaddpd and vaddps. I'll close this PR.

jlb6740 requested review from a team as code owners April 29, 2025 01:34

jlb6740 requested review from abrown and alexcrichton and removed request for a team April 29, 2025 01:34

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 Apr 29, 2025

jlb6740 force-pushed the add-initial-vex-encoding-support-1-of-2 branch 5 times, most recently from cb9474d to b41bba8 Compare April 30, 2025 18:40

Initial support for vex encoding for the new assembler.

b41bba8

Patch is first, one or two more will be pushed for enabling.

alexcrichton reviewed Apr 30, 2025

View reviewed changes

Remove dependency on meta crate from vex support

d962f1c

Also removes unneeded code and code cleans-up extraneous comments.

abrown reviewed May 1, 2025

View reviewed changes

jlb6740 closed this May 8, 2025

jlb6740 mentioned this pull request May 8, 2025

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jlb6740 commented Apr 29, 2025

alexcrichton Apr 30, 2025

cfallin Apr 30, 2025

jlb6740 Apr 30, 2025

jlb6740 Apr 30, 2025

abrown left a comment

abrown May 1, 2025

jlb6740 May 1, 2025

abrown May 1, 2025

jlb6740 May 1, 2025

abrown May 1, 2025

abrown May 1, 2025

jlb6740 May 1, 2025 •

edited

Loading

abrown May 1, 2025

abrown May 1, 2025

abrown May 1, 2025

jlb6740 May 1, 2025 •

edited

Loading

abrown May 1, 2025

abrown May 1, 2025

jlb6740 May 1, 2025

abrown May 1, 2025

jlb6740 commented May 8, 2025

	// test a single0 input, append `.seed(0x<failing seed>)`.
	// test a single input, append `.seed(0x<failing seed>)`.

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

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

Conversation

jlb6740 commented Apr 29, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 May 1, 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

jlb6740 May 1, 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

jlb6740 commented May 8, 2025

jlb6740 May 1, 2025 •

edited

Loading

jlb6740 May 1, 2025 •

edited

Loading