x64: use Rust types for assembler immediates (#10302)

abrown · web-flow · commit 469479aae6c7 · 2025-03-03T18:53:11.000Z
Previously we used `AssemblerImm*` and `AssemblerSimm*` throughout the
x64 ISLE to indicate the type of immediate an instruction would receive.
Alex has noted previously that this is unnecessary; it does after all
create more ties between the `cranelift-codegen` ISLE and
`cranelift-assembler-x64` that could possibly break in the future. This
change removes those ties by using Rust types in ISLE (e.g., `u8`, `i8`,
`u16`, etc.) and converting to the expected assembler type in the ISLE
glue layer.
diff --git a/cranelift/assembler-x64/meta/src/generate.rs b/cranelift/assembler-x64/meta/src/generate.rs
@@ -52,23 +52,6 @@ pub fn isle_macro(f: &mut Formatter, insts: &[dsl::Inst]) {
 /// Generate the ISLE definitions that match the `isle_assembler_methods!` macro
 /// above.
 pub fn isle_definitions(f: &mut Formatter, insts: &[dsl::Inst]) {
-    f.line("(type AssemblerImm8 extern (enum))", None);
-    f.line("(type AssemblerSimm8 extern (enum))", None);
-    f.line("(type AssemblerImm16 extern (enum))", None);
-    f.line("(type AssemblerSimm16 extern (enum))", None);
-    f.line("(type AssemblerImm32 extern (enum))", None);
-    f.line("(type AssemblerSimm32 extern (enum))", None);
-    f.line("(type AssemblerReadGpr extern (enum))", None);
-    f.line("(type AssemblerReadWriteGpr extern (enum))", None);
-    f.line("(type AssemblerReadGprMem extern (enum))", None);
-    f.line("(type AssemblerReadWriteGprMem extern (enum))", None);
-    f.line("(type AssemblerInst extern (enum))", None);
-    f.line("(type AssemblerReadXmm extern (enum))", None);
-    f.line("(type AssemblerReadWriteXmm extern (enum))", None);
-    f.line("(type AssemblerReadXmmMem extern (enum))", None);
-    f.line("(type AssemblerReadWriteXmmMem extern (enum))", None);
-    f.empty_line();
-
     f.line("(type AssemblerOutputs (enum", None);
     f.line("    ;; Used for instructions that have ISLE `SideEffect`s (memory stores, traps,", None);
     f.line("    ;; etc.) and do not return a `Value`.", None);
diff --git a/cranelift/assembler-x64/meta/src/generate/operand.rs b/cranelift/assembler-x64/meta/src/generate/operand.rs
@@ -26,45 +26,16 @@ impl dsl::Operand {
         }
     }
 
-    #[must_use]
-    pub fn generate_mut_ty(&self, read_ty: &str, read_write_ty: &str) -> Option<String> {
-        use dsl::Mutability::*;
-        use dsl::OperandKind::*;
-        let pick_ty = match self.mutability {
-            Read => read_ty,
-            ReadWrite => read_write_ty,
-        };
-        match self.location.kind() {
-            FixedReg(_) => None,
-            Imm(loc) => {
-                let bits = loc.bits();
-                if self.extension.is_sign_extended() {
-                    Some(format!("Simm{bits}"))
-                } else {
-                    Some(format!("Imm{bits}"))
-                }
-            }
-            Reg(r) => match r.bits() {
-                128 => Some(format!("Xmm<{pick_ty}>")),
-                _ => Some(format!("Gpr<{pick_ty}>")),
-            },
-            RegMem(rm) => match rm.bits() {
-                128 => Some(format!("XmmMem<{pick_ty}, Gpr>")),
-                _ => Some(format!("GprMem<{pick_ty}, {read_ty}>")),
-            },
-        }
-    }
-
-    /// Returns the type of this operand in ISLE as part of the
-    /// `IsleConstructorRaw` variants.
+    /// Returns the type of this operand in ISLE as a part of the ISLE "raw"
+    /// constructors.
     pub fn isle_param_raw(&self) -> String {
         match self.location.kind() {
             OperandKind::Imm(loc) => {
                 let bits = loc.bits();
                 if self.extension.is_sign_extended() {
-                    format!("AssemblerSimm{bits}")
+                    format!("i{bits}")
                 } else {
-                    format!("AssemblerImm{bits}")
+                    format!("u{bits}")
                 }
             }
             OperandKind::Reg(r) => match r.bits() {
@@ -104,9 +75,9 @@ impl dsl::Operand {
             OperandKind::Imm(loc) => {
                 let bits = loc.bits();
                 if self.extension.is_sign_extended() {
-                    format!("&cranelift_assembler_x64::Simm{bits}")
+                    format!("i{bits}")
                 } else {
-                    format!("&cranelift_assembler_x64::Imm{bits}")
+                    format!("u{bits}")
                 }
             }
             OperandKind::RegMem(rm) => match rm.bits() {
@@ -126,52 +97,33 @@ impl dsl::Operand {
     /// Effectively converts `self.rust_param_raw()` to the assembler type.
     pub fn rust_convert_isle_to_assembler(&self) -> Option<&'static str> {
         match self.location.kind() {
-            OperandKind::Reg(r) => match r.bits() {
-                128 => Some(match self.mutability {
-                    Mutability::Read => "cranelift_assembler_x64::Xmm::new",
-                    Mutability::ReadWrite => "self.convert_xmm_to_assembler_read_write_xmm",
-                }),
-                _ => Some(match self.mutability {
-                    Mutability::Read => "cranelift_assembler_x64::Gpr::new",
-                    Mutability::ReadWrite => "self.convert_gpr_to_assembler_read_write_gpr",
-                }),
-            },
-            OperandKind::RegMem(rm) => match rm.bits() {
-                128 => Some(match self.mutability {
-                    Mutability::Read => "self.convert_xmm_mem_to_assembler_read_xmm_mem",
-                    Mutability::ReadWrite => "self.convert_xmm_mem_to_assembler_read_write_xmm_mem",
-                }),
-                _ => Some(match self.mutability {
-                    Mutability::Read => "self.convert_gpr_mem_to_assembler_read_gpr_mem",
-                    Mutability::ReadWrite => "self.convert_gpr_mem_to_assembler_read_write_gpr_mem",
-                }),
+            OperandKind::Reg(r) => Some(match (r.bits(), self.mutability) {
+                (128, Mutability::Read) => "cranelift_assembler_x64::Xmm::new",
+                (128, Mutability::ReadWrite) => "self.convert_xmm_to_assembler_read_write_xmm",
+                (_, Mutability::Read) => "cranelift_assembler_x64::Gpr::new",
+                (_, Mutability::ReadWrite) => "self.convert_gpr_to_assembler_read_write_gpr",
+            }),
+            OperandKind::RegMem(r) => Some(match (r.bits(), self.mutability) {
+                (128, Mutability::Read) => "self.convert_xmm_mem_to_assembler_read_xmm_mem",
+                (128, Mutability::ReadWrite) => "self.convert_xmm_mem_to_assembler_read_write_xmm_mem",
+                (_, Mutability::Read) => "self.convert_gpr_mem_to_assembler_read_gpr_mem",
+                (_, Mutability::ReadWrite) => "self.convert_gpr_mem_to_assembler_read_write_gpr_mem",
+            }),
+            OperandKind::Imm(loc) => match (self.extension.is_sign_extended(), loc.bits()) {
+                (true, 8) => Some("cranelift_assembler_x64::Simm8::new"),
+                (true, 16) => Some("cranelift_assembler_x64::Simm16::new"),
+                (true, 32) => Some("cranelift_assembler_x64::Simm32::new"),
+                (false, 8) => Some("cranelift_assembler_x64::Imm8::new"),
+                (false, 16) => Some("cranelift_assembler_x64::Imm16::new"),
+                (false, 32) => Some("cranelift_assembler_x64::Imm32::new"),
+                _ => None,
             },
-            OperandKind::FixedReg(_) | OperandKind::Imm(_) => None,
+            OperandKind::FixedReg(_) => None,
         }
     }
 }
 
 impl dsl::Location {
-    /// `<operand type>`, if the operand has a type (i.e., not fixed registers).
-    #[must_use]
-    pub fn generate_type(&self, generic: Option<String>) -> Option<String> {
-        use dsl::Location::*;
-        let generic = match generic {
-            Some(ty) => format!("<{ty}>"),
-            None => String::new(),
-        };
-        match self {
-            al | ax | eax | rax | cl => None,
-            imm8 => Some("Imm8".into()),
-            imm16 => Some("Imm16".into()),
-            imm32 => Some("Imm32".into()),
-            r8 | r16 | r32 | r64 => Some(format!("Gpr{generic}")),
-            rm8 | rm16 | rm32 | rm64 => Some(format!("GprMem{generic}")),
-            xmm => Some(format!("Xmm{generic}")),
-            rm128 => Some(format!("XmmMem{generic}")),
-        }
-    }
-
     /// `self.<operand>.to_string(...)`
     #[must_use]
     pub fn generate_to_string(&self, extension: dsl::Extension) -> String {
@@ -200,7 +152,7 @@ impl dsl::Location {
 
     /// `Size::<operand size>`
     #[must_use]
-    pub fn generate_size(&self) -> Option<&str> {
+    fn generate_size(&self) -> Option<&str> {
         use dsl::Location::*;
         match self {
             al | ax | eax | rax | cl | imm8 | imm16 | imm32 => None,
diff --git a/cranelift/codegen/src/isa/x64/inst.isle b/cranelift/codegen/src/isa/x64/inst.isle
@@ -827,6 +827,8 @@
        ;; An instruction assembled outside of cranelift-codegen.
        (External (inst AssemblerInst))))
 
+(type AssemblerInst extern (enum))
+
 (type OperandSize extern
       (enum Size8
             Size16
@@ -2946,17 +2948,17 @@
                  dst_lo)))
 
 ;; Helpers for matching operands, extracting them into their assembler types.
-(decl is_imm8 (AssemblerImm8) GprMemImm)
+(decl is_imm8 (u8) GprMemImm)
 (extern extractor is_imm8 is_imm8)
-(decl is_simm8 (AssemblerSimm8) GprMemImm)
+(decl is_simm8 (i8) GprMemImm)
 (extern extractor is_simm8 is_simm8)
-(decl is_imm16 (AssemblerImm16) GprMemImm)
+(decl is_imm16 (u16) GprMemImm)
 (extern extractor is_imm16 is_imm16)
-(decl is_simm16 (AssemblerSimm16) GprMemImm)
+(decl is_simm16 (i16) GprMemImm)
 (extern extractor is_simm16 is_simm16)
-(decl is_imm32 (AssemblerImm32) GprMemImm)
+(decl is_imm32 (u32) GprMemImm)
 (extern extractor is_imm32 is_imm32)
-(decl is_simm32 (AssemblerSimm32) GprMemImm)
+(decl is_simm32 (i32) GprMemImm)
 (extern extractor is_simm32 is_simm32)
 (decl is_gpr (Gpr) GprMemImm)
 (extern extractor is_gpr is_gpr)
@@ -2967,12 +2969,6 @@
 (decl is_xmm (Xmm) XmmMem)
 (extern extractor is_xmm is_xmm)
 
-;; Helpers to auto-convert to and from assembler types.
-
-(decl u8_to_assembler_imm8 (u8) AssemblerImm8)
-(extern constructor u8_to_assembler_imm8 u8_to_assembler_imm8)
-(convert u8 AssemblerImm8 u8_to_assembler_imm8)
-
 ;; Helper for emitting `and` instructions.
 (decl x64_and (Type Gpr GprMemImm) Gpr)
 
diff --git a/cranelift/codegen/src/isa/x64/lower/isle.rs b/cranelift/codegen/src/isa/x64/lower/isle.rs
@@ -42,12 +42,6 @@ type BoxSyntheticAmode = Box<SyntheticAmode>;
 /// When interacting with the external assembler (see `external.rs`), we
 /// need to fix the types we'll use.
 type AssemblerInst = asm::Inst<CraneliftRegisters>;
-type AssemblerImm8 = asm::Imm8;
-type AssemblerSimm8 = asm::Simm8;
-type AssemblerImm16 = asm::Imm16;
-type AssemblerSimm16 = asm::Simm16;
-type AssemblerImm32 = asm::Imm32;
-type AssemblerSimm32 = asm::Simm32;
 
 pub struct SinkableLoad {
     inst: Inst,
@@ -958,56 +952,44 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> {
     ///// External assembler methods.
     ////////////////////////////////////////////////////////////////////////////
 
-    fn is_imm8(&mut self, src: &GprMemImm) -> Option<AssemblerImm8> {
+    fn is_imm8(&mut self, src: &GprMemImm) -> Option<u8> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => {
-                let imm = u8::try_from(simm32).ok()?;
-                Some(AssemblerImm8::new(imm))
-            }
+            RegMemImm::Imm { simm32 } => Some(u8::try_from(simm32).ok()?),
             _ => None,
         }
     }
 
-    fn is_simm8(&mut self, src: &GprMemImm) -> Option<AssemblerSimm8> {
+    fn is_simm8(&mut self, src: &GprMemImm) -> Option<i8> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => {
-                let imm = i8::try_from(simm32).ok()?;
-                Some(AssemblerSimm8::new(imm))
-            }
+            RegMemImm::Imm { simm32 } => Some(i8::try_from(simm32).ok()?),
             _ => None,
         }
     }
 
-    fn is_imm16(&mut self, src: &GprMemImm) -> Option<AssemblerImm16> {
+    fn is_imm16(&mut self, src: &GprMemImm) -> Option<u16> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => {
-                let imm = u16::try_from(simm32).ok()?;
-                Some(AssemblerImm16::new(imm))
-            }
+            RegMemImm::Imm { simm32 } => Some(u16::try_from(simm32).ok()?),
             _ => None,
         }
     }
 
-    fn is_simm16(&mut self, src: &GprMemImm) -> Option<AssemblerSimm16> {
+    fn is_simm16(&mut self, src: &GprMemImm) -> Option<i16> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => {
-                let imm = i16::try_from(simm32).ok()?;
-                Some(AssemblerSimm16::new(imm))
-            }
+            RegMemImm::Imm { simm32 } => Some(i16::try_from(simm32).ok()?),
             _ => None,
         }
     }
 
-    fn is_imm32(&mut self, src: &GprMemImm) -> Option<AssemblerImm32> {
+    fn is_imm32(&mut self, src: &GprMemImm) -> Option<u32> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => Some(AssemblerImm32::new(simm32)),
+            RegMemImm::Imm { simm32 } => Some(simm32),
             _ => None,
         }
     }
 
-    fn is_simm32(&mut self, src: &GprMemImm) -> Option<AssemblerSimm32> {
+    fn is_simm32(&mut self, src: &GprMemImm) -> Option<i32> {
         match src.clone().to_reg_mem_imm() {
-            RegMemImm::Imm { simm32 } => Some(AssemblerSimm32::new(simm32 as i32)),
+            RegMemImm::Imm { simm32 } => Some(simm32 as i32),
             _ => None,
         }
     }
@@ -1040,10 +1022,6 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> {
             RegMem::Mem { addr } => XmmMem::new(RegMem::Mem { addr }),
         }
     }
-
-    fn u8_to_assembler_imm8(&mut self, val: u8) -> AssemblerImm8 {
-        AssemblerImm8::new(val)
-    }
 }
 
 impl IsleContext<'_, '_, MInst, X64Backend> {
