Don't model a trap code as an operand

alexcrichton · alexcrichton · commit d87aecb9a0eb · 2025-05-21T18:08:41.000-07:00
diff --git a/cranelift/assembler-x64/meta/src/dsl.rs b/cranelift/assembler-x64/meta/src/dsl.rs
@@ -14,7 +14,7 @@ pub use encoding::{
 pub use encoding::{rex, vex};
 pub use features::{ALL_FEATURES, Feature, Features};
 pub use format::{Extension, Format, Location, Mutability, Operand, OperandKind, RegClass};
-pub use format::{align, fmt, implicit, r, rw, sxl, sxq, sxw, trap, w};
+pub use format::{align, fmt, implicit, r, rw, sxl, sxq, sxw, w};
 
 /// Abbreviated constructor for an x64 instruction.
 pub fn inst(
@@ -30,6 +30,7 @@ pub fn inst(
         format,
         encoding,
         features: features.into(),
+        has_trap: false,
     }
 }
 
@@ -56,6 +57,9 @@ pub struct Inst {
     /// "64-bit/32-bit Mode Support" and "CPUID Feature Flag" columns of the x64
     /// reference manual.
     pub features: Features,
+    /// Whether or not this instruction can trap and thus needs a `TrapCode`
+    /// payload in the instruction itself.
+    pub has_trap: bool,
 }
 
 impl Inst {
@@ -77,6 +81,13 @@ impl Inst {
             self.format.name.to_lowercase()
         )
     }
+
+    /// Flags this instruction as being able to trap, so needs a `TrapCode` at
+    /// compile time to track this.
+    pub fn has_trap(mut self) -> Self {
+        self.has_trap = true;
+        self
+    }
 }
 
 impl core::fmt::Display for Inst {
@@ -86,11 +97,15 @@ impl core::fmt::Display for Inst {
             format,
             encoding,
             features,
+            has_trap,
         } = self;
         write!(f, "{name}: {format} => {encoding}")?;
         if !features.is_empty() {
             write!(f, " [{features}]")?;
         }
+        if *has_trap {
+            write!(f, " has_trap")?;
+        }
         Ok(())
     }
 }
diff --git a/cranelift/assembler-x64/meta/src/dsl/encoding.rs b/cranelift/assembler-x64/meta/src/dsl/encoding.rs
@@ -205,17 +205,16 @@ impl Rex {
             assert!(
                 operands.iter().all(|&op| matches!(
                     op.location.kind(),
-                    Some(OperandKind::Imm(_) | OperandKind::FixedReg(_))
+                    OperandKind::Imm(_) | OperandKind::FixedReg(_)
                 ) || op.location.bits() == 16
-                    || op.location.bits() == 128
-                    || op.location.bits() == 0),
+                    || op.location.bits() == 128),
                 "when we encode the 66 prefix, we expect all operands to be 16-bit wide"
             );
         }
 
         if let Some(OperandKind::Imm(op)) = operands
             .iter()
-            .filter_map(|o| o.location.kind())
+            .map(|o| o.location.kind())
             .find(|k| matches!(k, OperandKind::Imm(_)))
         {
             assert_eq!(
diff --git a/cranelift/assembler-x64/meta/src/dsl/format.rs b/cranelift/assembler-x64/meta/src/dsl/format.rs
@@ -36,7 +36,7 @@ pub fn fmt(
 #[must_use]
 pub fn rw(op: impl Into<Operand>) -> Operand {
     let op = op.into();
-    assert!(!matches!(op.location.kind().unwrap(), OperandKind::Imm(_)));
+    assert!(!matches!(op.location.kind(), OperandKind::Imm(_)));
     Operand {
         mutability: Mutability::ReadWrite,
         ..op
@@ -73,25 +73,13 @@ pub fn align(location: Location) -> Operand {
 /// An abbreviated constructor for an operand that is used by the instruction
 /// but not visible in its disassembly.
 pub fn implicit(location: Location) -> Operand {
-    assert!(matches!(location.kind().unwrap(), OperandKind::FixedReg(_)));
+    assert!(matches!(location.kind(), OperandKind::FixedReg(_)));
     Operand {
         implicit: true,
         ..Operand::from(location)
     }
 }
 
-/// Creates an operand which represents a `TrapCode`.
-///
-/// This is not actually emitted as part of an instruction, but it's passed
-/// along to the `CodeSink` during emission at runtime.
-#[must_use]
-pub fn trap() -> Operand {
-    Operand {
-        implicit: true,
-        ..Operand::from(Location::trap)
-    }
-}
-
 /// An abbreviated constructor for a "read" operand that is sign-extended to 64
 /// bits (quadword).
 ///
@@ -167,12 +155,6 @@ impl Format {
         self.locations().copied().find(Location::uses_memory)
     }
 
-    /// Return the location of the operand that represents an explicit trap.
-    pub fn explicit_trap(&self) -> Option<Location> {
-        debug_assert!(self.locations().copied().filter(Location::is_trap).count() <= 1);
-        self.locations().copied().find(Location::is_trap)
-    }
-
     /// Return `true` if any of the operands accepts a register (i.e., not an
     /// immediate); return `false` otherwise.
     #[must_use]
@@ -181,13 +163,8 @@ impl Format {
     }
 
     /// Collect into operand kinds.
-    ///
-    /// Note that this will drop operands which do not have an `OperandKind`
-    /// associated with them, such as trap codes. This is mainly useful when
-    /// looking at the structure of an instruction and considering what's going
-    /// to get emitted for the instruction.
     pub fn operands_by_kind(&self) -> Vec<OperandKind> {
-        self.locations().filter_map(Location::kind).collect()
+        self.locations().map(Location::kind).collect()
     }
 }
 
@@ -312,9 +289,6 @@ pub enum Location {
     imm16,
     imm32,
 
-    // Compiler-specific trap code for this instruction.
-    trap,
-
     // General-purpose registers, and their memory forms.
     r8,
     r16,
@@ -349,10 +323,6 @@ impl Location {
             eax | edx | imm32 | r32 | rm32 | m32 | xmm_m32 => 32,
             rax | rdx | r64 | rm64 | m64 | xmm_m64 => 64,
             xmm | xmm_m128 => 128,
-
-            // not present in the instruction stream, this is just compiler
-            // metadata.
-            trap => 0,
         }
     }
 
@@ -367,50 +337,35 @@ impl Location {
     pub fn uses_memory(&self) -> bool {
         use Location::*;
         match self {
-            al | ax | eax | rax | cl | dx | edx | rdx | imm8 | imm16 | imm32 | trap | r8 | r16
-            | r32 | r64 | xmm => false,
+            al | ax | eax | rax | cl | dx | edx | rdx | imm8 | imm16 | imm32 | r8 | r16 | r32
+            | r64 | xmm => false,
             rm8 | rm16 | rm32 | rm64 | xmm_m32 | xmm_m64 | xmm_m128 | m8 | m16 | m32 | m64 => true,
         }
     }
 
-    /// Return `true` if the location represents a compiler trap code.
-    #[must_use]
-    pub fn is_trap(&self) -> bool {
-        use Location::*;
-        match self {
-            trap => true,
-            _ => false,
-        }
-    }
-
     /// Return `true` if any of the operands accepts a register (i.e., not an
     /// immediate); return `false` otherwise.
     #[must_use]
     pub fn uses_register(&self) -> bool {
         use Location::*;
         match self {
-            imm8 | imm16 | imm32 | trap => false,
+            imm8 | imm16 | imm32 => false,
             al | ax | eax | rax | cl | dx | edx | rdx | r8 | r16 | r32 | r64 | rm8 | rm16
             | rm32 | rm64 | xmm | xmm_m32 | xmm_m64 | xmm_m128 | m8 | m16 | m32 | m64 => true,
         }
     }
 
     /// Convert the location to an [`OperandKind`].
-    ///
-    /// Returns `Some` if this operand is part of an emitted instruction, and
-    /// `None` is returned if the operand is only for compiler metadata (e.g. a
-    /// trap code).
     #[must_use]
-    pub fn kind(&self) -> Option<OperandKind> {
+    pub fn kind(&self) -> OperandKind {
         use Location::*;
-        Some(match self {
+        match self {
             al | ax | eax | rax | cl | dx | edx | rdx => OperandKind::FixedReg(*self),
             imm8 | imm16 | imm32 => OperandKind::Imm(*self),
             r8 | r16 | r32 | r64 | xmm => OperandKind::Reg(*self),
             rm8 | rm16 | rm32 | rm64 | xmm_m32 | xmm_m64 | xmm_m128 => OperandKind::RegMem(*self),
             m8 | m16 | m32 | m64 => OperandKind::Mem(*self),
-            trap => return None,
-        })
+        }
     }
 
     /// If a location directly uses data from a register, return the register
@@ -421,7 +376,7 @@ impl Location {
     pub fn reg_class(&self) -> Option<RegClass> {
         use Location::*;
         match self {
-            imm8 | imm16 | imm32 | trap | m8 | m16 | m32 | m64 => None,
+            imm8 | imm16 | imm32 | m8 | m16 | m32 | m64 => None,
             al | ax | eax | rax | cl | dx | edx | rdx | r8 | r16 | r32 | r64 | rm8 | rm16
             | rm32 | rm64 => Some(RegClass::Gpr),
             xmm | xmm_m32 | xmm_m64 | xmm_m128 => Some(RegClass::Xmm),
@@ -436,7 +391,6 @@ impl core::fmt::Display for Location {
             imm8 => write!(f, "imm8"),
             imm16 => write!(f, "imm16"),
             imm32 => write!(f, "imm32"),
-            trap => write!(f, "trap"),
 
             al => write!(f, "al"),
             ax => write!(f, "ax"),
@@ -473,7 +427,7 @@ impl core::fmt::Display for Location {
 ///
 /// ```
 /// # use cranelift_assembler_x64_meta::dsl::{OperandKind, Location};
-/// let k: OperandKind = Location::imm32.kind().unwrap();
+/// let k: OperandKind = Location::imm32.kind();
 /// ```
 #[derive(Clone, Copy, Debug)]
 pub enum OperandKind {
diff --git a/cranelift/assembler-x64/meta/src/generate/format.rs b/cranelift/assembler-x64/meta/src/generate/format.rs
@@ -87,7 +87,7 @@ impl dsl::Format {
         f.comment("Possibly emit REX prefix.");
 
         let find_8bit_registers =
-            |l: &dsl::Location| l.bits() == 8 && matches!(l.kind(), Some(Reg(_) | RegMem(_)));
+            |l: &dsl::Location| l.bits() == 8 && matches!(l.kind(), Reg(_) | RegMem(_));
         let uses_8bit = self.locations().any(find_8bit_registers);
         fmtln!(f, "let uses_8bit = {uses_8bit};");
         fmtln!(f, "let w_bit = {};", rex.w);
diff --git a/cranelift/assembler-x64/meta/src/generate/inst.rs b/cranelift/assembler-x64/meta/src/generate/inst.rs
@@ -22,6 +22,10 @@ impl dsl::Inst {
                 let ty = k.generate_type();
                 fmtln!(f, "pub {loc}: {ty},");
             }
+
+            if self.has_trap {
+                fmtln!(f, "pub trap: TrapCode,");
+            }
         });
     }
 
@@ -69,7 +73,12 @@ impl dsl::Inst {
             self.format
                 .operands
                 .iter()
-                .map(|o| format!("{}: impl Into<{}>", o.location, o.generate_type())),
+                .map(|o| format!("{}: impl Into<{}>", o.location, o.generate_type()))
+                .chain(if self.has_trap {
+                    Some("trap: impl Into<TrapCode>".to_string())
+                } else {
+                    None
+                }),
         );
         fmtln!(f, "#[must_use]");
         f.add_block(&format!("pub fn new({params}) -> Self"), |f| {
@@ -78,6 +87,9 @@ impl dsl::Inst {
                     let loc = o.location;
                     fmtln!(f, "{loc}: {loc}.into(),");
                 }
+                if self.has_trap {
+                    fmtln!(f, "trap: trap.into(),");
+                }
             });
         });
     }
@@ -98,7 +110,7 @@ impl dsl::Inst {
                 if let Some(op) = self.format.uses_memory() {
                     use dsl::OperandKind::*;
                     f.comment("Emit trap.");
-                    match op.kind().unwrap() {
+                    match op.kind() {
                         Mem(_) => {
                             f.add_block(
                                 &format!("if let Some(trap_code) = self.{op}.trap_code()"),
@@ -121,9 +133,9 @@ impl dsl::Inst {
                         _ => unreachable!(),
                     }
                 }
-                if let Some(op) = self.format.explicit_trap() {
+                if self.has_trap {
                     f.comment("Emit trap.");
-                    fmtln!(f, "buf.add_trap(self.{op});");
+                    fmtln!(f, "buf.add_trap(self.trap);");
                 }
 
                 match &self.encoding {
@@ -147,33 +159,31 @@ impl dsl::Inst {
                 let mutability = o.mutability.generate_snake_case();
                 let reg = o.location.reg_class();
                 match o.location.kind() {
-                    Some(Imm(_)) => {
+                    Imm(_) => {
                         // Immediates do not need register allocation.
                     }
-                    Some(FixedReg(loc)) => {
+                    FixedReg(loc) => {
                         let reg_lower = reg.unwrap().to_string().to_lowercase();
                         fmtln!(f, "let enc = self.{loc}.expected_enc();");
                         fmtln!(f, "visitor.fixed_{mutability}_{reg_lower}(&mut self.{loc}.0, enc);");
                     }
-                    Some(Reg(loc)) => {
+                    Reg(loc) => {
                         let reg_lower = reg.unwrap().to_string().to_lowercase();
                         fmtln!(f, "visitor.{mutability}_{reg_lower}(self.{loc}.as_mut());");
                     }
-                    Some(RegMem(loc) )=> {
+                    RegMem(loc) => {
                         let reg = reg.unwrap();
                         let reg_lower = reg.to_string().to_lowercase();
                         fmtln!(f, "visitor.{mutability}_{reg_lower}_mem(&mut self.{loc});");
                     }
-                    Some(Mem(loc) )=> {
+                    Mem(loc) => {
                         // Note that this is always "read" because from a
                         // regalloc perspective when using an amode it means
                         // that the while a write is happening that's to
                         // memory, not registers.
                         fmtln!(f, "visitor.read_amode(&mut self.{loc});");
                     }
 
-                    // this operand doesn't participate in register allocation
-                    None => {}
                 }
             }
         });
diff --git a/cranelift/assembler-x64/meta/src/generate/operand.rs b/cranelift/assembler-x64/meta/src/generate/operand.rs
@@ -14,7 +14,6 @@ impl dsl::Operand {
                     format!("Imm{bits}")
                 }
             }
-            trap => format!("TrapCode"),
             al | ax | eax | rax | cl | dx | edx | rdx => {
                 let enc = match self.location {
                     al | ax | eax | rax => "{ gpr::enc::RAX }",
@@ -49,9 +48,6 @@ impl dsl::Location {
                     format!("self.{self}.to_string()")
                 }
             }
-            trap => {
-                format!("self.{self}")
-            }
             al | ax | eax | rax | cl | dx | edx | rdx | r8 | r16 | r32 | r64 | rm8 | rm16
             | rm32 | rm64 => match self.generate_size() {
                 Some(size) => format!("self.{self}.to_string({size})"),
@@ -68,7 +64,7 @@ impl dsl::Location {
     fn generate_size(&self) -> Option<&str> {
         use dsl::Location::*;
         match self {
-            imm8 | imm16 | imm32 | trap => None,
+            imm8 | imm16 | imm32 => None,
             al | cl | r8 | rm8 => Some("Size::Byte"),
             ax | dx | r16 | rm16 => Some("Size::Word"),
             eax | edx | r32 | rm32 => Some("Size::Doubleword"),
diff --git a/cranelift/assembler-x64/meta/src/instructions/div.rs b/cranelift/assembler-x64/meta/src/instructions/div.rs
diff --git a/cranelift/codegen/meta/src/gen_asm.rs b/cranelift/codegen/meta/src/gen_asm.rs

Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ pub use encoding::{`
`14`	`14`	`pub use encoding::{rex, vex};`
`15`	`15`	`pub use features::{ALL_FEATURES, Feature, Features};`
`16`	`16`	`pub use format::{Extension, Format, Location, Mutability, Operand, OperandKind, RegClass};`
`17`		`-pub use format::{align, fmt, implicit, r, rw, sxl, sxq, sxw, trap, w};`
	`17`	`+pub use format::{align, fmt, implicit, r, rw, sxl, sxq, sxw, w};`
`18`	`18`
`19`	`19`	`/// Abbreviated constructor for an x64 instruction.`
`20`	`20`	`pub fn inst(`
`@@ -30,6 +30,7 @@ pub fn inst(`
`30`	`30`	`format,`
`31`	`31`	`encoding,`
`32`	`32`	`features: features.into(),`
	`33`	`+ has_trap: false,`
`33`	`34`	`}`
`34`	`35`	`}`
`35`	`36`
`@@ -56,6 +57,9 @@ pub struct Inst {`
`56`	`57`	`/// "64-bit/32-bit Mode Support" and "CPUID Feature Flag" columns of the x64`
`57`	`58`	`/// reference manual.`
`58`	`59`	`pub features: Features,`
	`60`	+ /// Whether or not this instruction can trap and thus needs a `TrapCode`
	`61`	`+ /// payload in the instruction itself.`
	`62`	`+ pub has_trap: bool,`
`59`	`63`	`}`
`60`	`64`
`61`	`65`	`impl Inst {`
`@@ -77,6 +81,13 @@ impl Inst {`
`77`	`81`	`self.format.name.to_lowercase()`
`78`	`82`	`)`
`79`	`83`	`}`
	`84`	`+`
	`85`	+ /// Flags this instruction as being able to trap, so needs a `TrapCode` at
	`86`	`+ /// compile time to track this.`
	`87`	`+ pub fn has_trap(mut self) -> Self {`
	`88`	`+ self.has_trap = true;`
	`89`	`+ self`
	`90`	`+ }`
`80`	`91`	`}`
`81`	`92`
`82`	`93`	`impl core::fmt::Display for Inst {`
`@@ -86,11 +97,15 @@ impl core::fmt::Display for Inst {`
`86`	`97`	`format,`
`87`	`98`	`encoding,`
`88`	`99`	`features,`
	`100`	`+ has_trap,`
`89`	`101`	`} = self;`
`90`	`102`	`write!(f, "{name}: {format} => {encoding}")?;`
`91`	`103`	`if !features.is_empty() {`
`92`	`104`	`write!(f, " [{features}]")?;`
`93`	`105`	`}`
	`106`	`+ if *has_trap {`
	`107`	`+ write!(f, " has_trap")?;`
	`108`	`+ }`
`94`	`109`	`Ok(())`
`95`	`110`	`}`
`96`	`111`	`}`