Minor optimizations to assembler-generated code (bytecodealliance#10811)

* Minor optimizations to assembler-generated code

This is a few minor changes in the hopes of optimizing the compile-time
of the generated code itself for the x64 assembler crate, including:

* Add `derive(Copy)` to all instructions to benefit from a specialized
  implementation of `derive(Clone)` when a `Copy` implementation is also
  present (e.g. it's `*foo` instead of a structural `clone`-each-field).
* Don't use `write!` in `Display for Inst` and instead delegate directly
  with methods to avoid formatting machinery.
* Use helper methods in `RegisterVisitor` trait to have register
  allocation be a method-per-operand and shrink the methods a bit.

* Fix clippy warnings

Author: alexcrichton

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

@@ -47,7 +47,7 @@ fn generate_inst_enum(f: &mut Formatter, insts: &[dsl::Inst]) {
 
 /// `#[derive(...)]`
 fn generate_derive(f: &mut Formatter) {
-    fmtln!(f, "#[derive(Clone, Debug)]");
+    fmtln!(f, "#[derive(Copy, Clone, Debug)]");
     fmtln!(
         f,
         "#[cfg_attr(any(test, feature = \"fuzz\"), derive(arbitrary::Arbitrary))]"
@@ -72,7 +72,7 @@ fn generate_inst_display_impl(f: &mut Formatter, insts: &[dsl::Inst]) {
                 f.add_block("match self", |f| {
                     for inst in insts {
                         let variant_name = inst.name();
-                        fmtln!(f, "Self::{variant_name}(i) => write!(f, \"{{i}}\"),");
+                        fmtln!(f, "Self::{variant_name}(i) => i.fmt(f),");
                     }
                 });
             },

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

@@ -11,7 +11,7 @@ impl dsl::Feature {
     pub fn generate_enum(f: &mut Formatter) {
         fmtln!(f, "#[doc(hidden)]");
         generate_derive(f);
-        fmtln!(f, "#[derive(Copy, PartialEq)]"); // Add a couple more helpful derives.
+        fmtln!(f, "#[derive(PartialEq)]"); // Add more helpful derives.
         f.add_block("pub enum Feature", |f| {
             for feature in dsl::ALL_FEATURES {
                 fmtln!(f, "{feature},");

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

@@ -158,13 +158,14 @@ impl dsl::Inst {
                     RegMem(loc) => {
                         let reg = reg.unwrap();
                         let reg_lower = reg.to_string().to_lowercase();
-                        f.add_block(&format!("match &mut self.{loc}"), |f| {
-                            fmtln!(f, "{reg}Mem::{reg}(r) => visitor.{mutability}_{reg_lower}(r),");
-                            fmtln!(f, "{reg}Mem::Mem(m) => visit_amode(m, visitor),");
-                        });
+                        fmtln!(f, "visitor.{mutability}_{reg_lower}_mem(&mut self.{loc});");
                     }
                     Mem(loc) => {
-                        fmtln!(f, "visit_amode(&mut self.{loc}, visitor);");
+                        // 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});");
                     }
                 }
             }

cranelift/assembler-x64/src/api.rs

@@ -2,6 +2,7 @@
 
 use crate::gpr;
 use crate::xmm;
+use crate::{Amode, GprMem, XmmMem};
 use std::{num::NonZeroU8, ops::Index, vec::Vec};
 
 /// Describe how an instruction is emitted into a code buffer.
@@ -67,12 +68,12 @@ impl CodeSink for Vec<u8> {
 }
 
 /// Wrap [`CodeSink`]-specific labels.
-#[derive(Debug, Clone)]
+#[derive(Debug, Copy, Clone)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Label(pub u32);
 
 /// Wrap [`CodeSink`]-specific constant keys.
-#[derive(Debug, Clone)]
+#[derive(Debug, Copy, Clone)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Constant(pub u32);
 
@@ -125,7 +126,7 @@ pub trait Registers {
 }
 
 /// Describe how to interact with an external register type.
-pub trait AsReg: Clone + std::fmt::Debug {
+pub trait AsReg: Copy + Clone + std::fmt::Debug {
     /// Create a register from its hardware encoding.
     ///
     /// This is primarily useful for fuzzing, though it is also useful for
@@ -194,4 +195,46 @@ pub trait RegisterVisitor<R: Registers> {
     /// Visit a read-only fixed SSE register; this register can be modified
     /// in-place but must emit as the hardware encoding `enc`.
     fn fixed_write_xmm(&mut self, reg: &mut R::WriteXmm, enc: u8);
+
+    /// Visit the registers in an [`Amode`].
+    ///
+    /// This is helpful for generated code: it allows capturing the `R::ReadGpr`
+    /// type (which an `Amode` method cannot) and simplifies the code to be
+    /// generated.
+    fn read_amode(&mut self, amode: &mut Amode<R::ReadGpr>) {
+        match amode {
+            Amode::ImmReg { base, .. } => {
+                self.read_gpr(base);
+            }
+            Amode::ImmRegRegShift { base, index, .. } => {
+                self.read_gpr(base);
+                self.read_gpr(index.as_mut());
+            }
+            Amode::RipRelative { .. } => {}
+        }
+    }
+
+    /// Helper method to handle a read/write [`GprMem`] operand.
+    fn read_write_gpr_mem(&mut self, op: &mut GprMem<R::ReadWriteGpr, R::ReadGpr>) {
+        match op {
+            GprMem::Gpr(r) => self.read_write_gpr(r),
+            GprMem::Mem(m) => self.read_amode(m),
+        }
+    }
+
+    /// Helper method to handle a read-only [`GprMem`] operand.
+    fn read_gpr_mem(&mut self, op: &mut GprMem<R::ReadGpr, R::ReadGpr>) {
+        match op {
+            GprMem::Gpr(r) => self.read_gpr(r),
+            GprMem::Mem(m) => self.read_amode(m),
+        }
+    }
+
+    /// Helper method to handle a read-only [`XmmMem`] operand.
+    fn read_xmm_mem(&mut self, op: &mut XmmMem<R::ReadXmm, R::ReadGpr>) {
+        match op {
+            XmmMem::Xmm(r) => self.read_xmm(r),
+            XmmMem::Mem(m) => self.read_amode(m),
+        }
+    }
 }

cranelift/assembler-x64/src/fixed.rs

@@ -22,7 +22,7 @@ use crate::AsReg;
 /// let fixed = Fixed::<u8, { gpr::enc::RAX }>(invalid_reg);
 /// fixed.enc(); // Will panic because `invalid_reg` does not match `RAX`.
 /// ```
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub struct Fixed<R, const E: u8>(pub R);
 
 impl<R, const E: u8> Fixed<R, E> {
@@ -52,3 +52,9 @@ impl<R, const E: u8> AsRef<R> for Fixed<R, E> {
         &self.0
     }
 }
+
+impl<R, const E: u8> From<R> for Fixed<R, E> {
+    fn from(reg: R) -> Fixed<R, E> {
+        Fixed(reg)
+    }
+}

cranelift/assembler-x64/src/imm.rs

@@ -107,7 +107,7 @@ impl TryFrom<i32> for Simm8 {
 }
 
 /// A 16-bit immediate operand.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Imm16(u16);
 
@@ -147,7 +147,7 @@ impl fmt::Display for Imm16 {
 }
 
 /// A _signed_ 16-bit immediate operand (suitable for sign extension).
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Simm16(i16);
 
@@ -196,7 +196,7 @@ impl TryFrom<i32> for Simm16 {
 /// Note that, "in 64-bit mode, the typical size of immediate operands remains
 /// 32 bits. When the operand size is 64 bits, the processor sign-extends all
 /// immediates to 64 bits prior to their use" (Intel SDM Vol. 2, 2.2.1.5).
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Imm32(u32);
 
@@ -240,7 +240,7 @@ impl fmt::Display for Imm32 {
 /// Note that, "in 64-bit mode, the typical size of immediate operands remains
 /// 32 bits. When the operand size is 64 bits, the processor sign-extends all
 /// immediates to 64 bits prior to their use" (Intel SDM Vol. 2, 2.2.1.5).
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Simm32(i32);
 

cranelift/assembler-x64/src/inst.rs

@@ -7,7 +7,7 @@ use crate::Fixed;
 use crate::api::{AsReg, CodeSink, KnownOffsetTable, RegisterVisitor, Registers};
 use crate::gpr::{self, Gpr, Size};
 use crate::imm::{Extension, Imm8, Imm16, Imm32, Simm8, Simm32};
-use crate::mem::{Amode, GprMem, XmmMem, visit_amode};
+use crate::mem::{Amode, GprMem, XmmMem};
 use crate::rex::RexPrefix;
 use crate::xmm::Xmm;
 

cranelift/assembler-x64/src/mem.rs

@@ -3,10 +3,9 @@
 use crate::api::{AsReg, CodeSink, Constant, KnownOffset, KnownOffsetTable, Label, TrapCode};
 use crate::gpr::{self, NonRspGpr, Size};
 use crate::rex::{Disp, RexPrefix, encode_modrm, encode_sib};
-use crate::{RegisterVisitor, Registers};
 
 /// x64 memory addressing modes.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub enum Amode<R: AsReg> {
     ImmReg {
@@ -62,27 +61,6 @@ impl<R: AsReg> Amode<R> {
     }
 }
 
-/// Visit the registers in an [`Amode`].
-///
-/// This is helpful for generated code: it allows capturing the `R::ReadGpr`
-/// type (which an `Amode` method cannot) and simplifies the code to be
-/// generated.
-pub(crate) fn visit_amode<R: Registers>(
-    amode: &mut Amode<R::ReadGpr>,
-    visitor: &mut impl RegisterVisitor<R>,
-) {
-    match amode {
-        Amode::ImmReg { base, .. } => {
-            visitor.read_gpr(base);
-        }
-        Amode::ImmRegRegShift { base, index, .. } => {
-            visitor.read_gpr(base);
-            visitor.read_gpr(index.as_mut());
-        }
-        Amode::RipRelative { .. } => {}
-    }
-}
-
 /// A 32-bit immediate for address offsets.
 #[derive(Clone, Copy, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
@@ -134,7 +112,7 @@ impl std::fmt::LowerHex for AmodeOffset {
 /// happens immediately before emission:
 /// - the [`KnownOffset`] is looked up, mapping it to an offset value
 /// - the [`Simm32`] value is added to the offset value
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub struct AmodeOffsetPlusKnownOffset {
     pub simm32: AmodeOffset,
     pub offset: Option<KnownOffset>,
@@ -166,7 +144,7 @@ impl std::fmt::LowerHex for AmodeOffsetPlusKnownOffset {
 }
 
 /// For RIP-relative addressing, keep track of the [`CodeSink`]-specific target.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub enum DeferredTarget {
     Label(Label),
@@ -205,7 +183,7 @@ impl<R: AsReg> std::fmt::Display for Amode<R> {
 }
 
 /// The scaling factor for the index register in certain [`Amode`]s.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub enum Scale {
     One,
@@ -252,7 +230,7 @@ impl Scale {
 }
 
 /// A general-purpose register or memory operand.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 #[allow(
     clippy::module_name_repetitions,
@@ -313,7 +291,7 @@ impl<R: AsReg, M: AsReg> From<Amode<M>> for GprMem<R, M> {
 }
 
 /// An XMM register or memory operand.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 #[allow(
     clippy::module_name_repetitions,
@@ -382,7 +360,7 @@ fn emit_modrm_sib_disp<R: AsReg>(
     bytes_at_end: u8,
     evex_scaling: Option<i8>,
 ) {
-    match mem_e.clone() {
+    match *mem_e {
         Amode::ImmReg { simm32, base, .. } => {
             let enc_e = base.enc();
             let mut imm = Disp::new(simm32.value(offsets), evex_scaling);
@@ -450,8 +428,8 @@ fn emit_modrm_sib_disp<R: AsReg>(
 
             let offset = sink.current_offset();
             let target = match target {
-                DeferredTarget::Label(label) => label.clone(),
-                DeferredTarget::Constant(constant) => sink.get_label_for_constant(constant.clone()),
+                DeferredTarget::Label(label) => label,
+                DeferredTarget::Constant(constant) => sink.get_label_for_constant(constant),
             };
             sink.use_label_at_offset(offset, target);