x64: Migrate xmm mov-family instructions to new assembler (#10834)

* x64: Migrate xmm mov-family instructions to new assembler

This commit migrates xmm movement-style instructions, such as
`mov{s{s,d},{a,u}p{s,d},dq{a,u}}` to the new assembler. This enables
deleting the `XmmMovRM` class of instructions in ISLE. Along the way a
number of notable changes were made:

* ISLE constructors for assembler instructions now take `SyntheticAmode`
  instead of `Amode` since it's already supported anyway and it's a more
  flexible argument to take.
* The conversion from `SyntheticAmode` to the assembler `Amode<Gpr>` was
  fixed where one variant needed to use `rsp` instead of `rbp`.
* The `is_move` method was updated to ignore `movss` and `movsd`
  instructions since they're not true movement-related instructions in
  register-to-register situations.

* Use `gen_move` helper in Winch

Author: alexcrichton

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

@@ -118,7 +118,7 @@ impl dsl::Format {
                 fmtln!(f, "let digit = 0x{digit:x};");
                 fmtln!(f, "let rex = self.{dst}.as_rex_prefix(digit, {bits});");
             }
-            [Reg(dst), RegMem(src)] | [Reg(dst), RegMem(src), Imm(_)] => {
+            [Reg(dst), RegMem(src)] | [Reg(dst), RegMem(src), Imm(_)] | [Reg(dst), Mem(src)] => {
                 fmtln!(f, "let dst = self.{dst}.enc();");
                 fmtln!(f, "let rex = self.{src}.as_rex_prefix(dst, {bits});");
             }
@@ -219,7 +219,8 @@ impl dsl::Format {
             | [Reg(reg), Reg(_), RegMem(mem)]
             | [RegMem(mem), Reg(reg)]
             | [RegMem(mem), Reg(reg), Imm(_)]
-            | [RegMem(mem), Reg(reg), FixedReg(_)] => {
+            | [RegMem(mem), Reg(reg), FixedReg(_)]
+            | [Reg(reg), Mem(mem)] => {
                 fmtln!(f, "let reg = self.{reg}.enc();");
                 fmtln!(
                     f,

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

@@ -8,6 +8,7 @@ mod div;
 mod lanes;
 mod max;
 mod min;
+mod mov;
 mod mul;
 mod neg;
 mod or;
@@ -30,6 +31,7 @@ pub fn list() -> Vec<Inst> {
     all.extend(lanes::list());
     all.extend(max::list());
     all.extend(min::list());
+    all.extend(mov::list());
     all.extend(mul::list());
     all.extend(neg::list());
     all.extend(or::list());

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

@@ -22,11 +22,6 @@ pub fn list() -> Vec<Inst> {
         inst("pinsrd", fmt("A", [rw(xmm1), r(rm32), r(imm8)]), rex([0x66, 0x0F, 0x3A, 0x22]).r().ib(), _64b | compat | sse41),
         inst("pinsrq", fmt("A", [rw(xmm1), r(rm64), r(imm8)]), rex([0x66, 0x0F, 0x3A, 0x22]).r().ib().w(), _64b | sse41),
 
-        inst("movd", fmt("A", [w(xmm1), r(rm32)]), rex([0x66, 0x0F, 0x6E]).r(), _64b | compat | sse2),
-        inst("movq", fmt("A", [w(xmm1), r(rm64)]), rex([0x66, 0x0F, 0x6E]).r().w(), _64b | sse2),
-        inst("movd", fmt("B", [w(rm32), r(xmm2)]), rex([0x66, 0x0F, 0x7E]).r(), _64b | compat | sse2),
-        inst("movq", fmt("B", [w(rm64), r(xmm2)]), rex([0x66, 0x0F, 0x7E]).r().w(), _64b | sse2),
-
         inst("movmskps", fmt("RM", [w(r32), r(xmm2)]), rex([0x0F, 0x50]).r(), _64b | compat | sse),
         inst("movmskpd", fmt("RM", [w(r32), r(xmm2)]), rex([0x66, 0x0F, 0x50]).r(), _64b | compat | sse2),
         inst("pmovmskb", fmt("RM", [w(r32), r(xmm2)]), rex([0x66, 0x0F, 0xD7]).r(), _64b | compat | sse2),

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

@@ -0,0 +1,44 @@
+use crate::dsl::{Feature::*, Inst, Location::*};
+use crate::dsl::{align, fmt, inst, r, rex, rw, w};
+
+#[rustfmt::skip] // Keeps instructions on a single line.
+pub fn list() -> Vec<Inst> {
+    vec![
+        inst("movd", fmt("A", [w(xmm1), r(rm32)]), rex([0x66, 0x0F, 0x6E]).r(), _64b | compat | sse2),
+        inst("movq", fmt("A", [w(xmm1), r(rm64)]), rex([0x66, 0x0F, 0x6E]).r().w(), _64b | sse2),
+        inst("movd", fmt("B", [w(rm32), r(xmm2)]), rex([0x66, 0x0F, 0x7E]).r(), _64b | compat | sse2),
+        inst("movq", fmt("B", [w(rm64), r(xmm2)]), rex([0x66, 0x0F, 0x7E]).r().w(), _64b | sse2),
+
+        // Note that `movss` and `movsd` only have an "A" and "C" modes listed
+        // in the Intel manual but here they're split into "*_M" and "*_R" to
+        // model the different regalloc behavior each one has. Notably the
+        // memory-using variant does the usual read or write the memory
+        // depending on the instruction, but the "*_R" variant both reads and
+        // writes the destination register because the upper bits are preserved.
+        //
+        // Additionally "C_R" is not specified here since it's not needed over
+        // the "A_R" variant and it's additionally not encoded correctly as the
+        // destination must be modeled in the ModRM:r/m byte, not the ModRM:reg
+        // byte. Currently our encoding based on format doesn't account for this
+        // special case, so it's just dropped here.
+        inst("movss", fmt("A_M", [w(xmm1), r(m32)]), rex([0xF3, 0x0F, 0x10]).r(), _64b | sse),
+        inst("movss", fmt("A_R", [rw(xmm1), r(xmm2)]), rex([0xF3, 0x0F, 0x10]).r(), _64b | sse),
+        inst("movss", fmt("C_M", [w(m64), r(xmm1)]), rex([0xF3, 0x0F, 0x11]).r(), _64b | sse),
+        inst("movsd", fmt("A_M", [w(xmm1), r(m32)]), rex([0xF2, 0x0F, 0x10]).r(), _64b | sse2),
+        inst("movsd", fmt("A_R", [rw(xmm1), r(xmm2)]), rex([0xF2, 0x0F, 0x10]).r(), _64b | sse2),
+        inst("movsd", fmt("C_M", [w(m64), r(xmm1)]), rex([0xF2, 0x0F, 0x11]).r(), _64b | sse2),
+
+        inst("movapd", fmt("A", [w(xmm1), r(align(xmm_m128))]), rex([0x66, 0x0F, 0x28]).r(), _64b | sse2),
+        inst("movapd", fmt("B", [w(align(xmm_m128)), r(xmm1)]), rex([0x66, 0x0F, 0x29]).r(), _64b | sse2),
+        inst("movaps", fmt("A", [w(xmm1), r(align(xmm_m128))]), rex([0x0F, 0x28]).r(), _64b | sse),
+        inst("movaps", fmt("B", [w(align(xmm_m128)), r(xmm1)]), rex([0x0F, 0x29]).r(), _64b | sse),
+        inst("movupd", fmt("A", [w(xmm1), r(xmm_m128)]), rex([0x66, 0x0F, 0x10]).r(), _64b | sse2),
+        inst("movupd", fmt("B", [w(xmm_m128), r(xmm1)]), rex([0x66, 0x0F, 0x11]).r(), _64b | sse2),
+        inst("movups", fmt("A", [w(xmm1), r(xmm_m128)]), rex([0x0F, 0x10]).r(), _64b | sse),
+        inst("movups", fmt("B", [w(xmm_m128), r(xmm1)]), rex([0x0F, 0x11]).r(), _64b | sse),
+        inst("movdqa", fmt("A", [w(xmm1), r(align(xmm_m128))]), rex([0x66, 0x0F, 0x6F]).r(), _64b | sse2),
+        inst("movdqa", fmt("B", [w(align(xmm_m128)), r(xmm1)]), rex([0x66, 0x0F, 0x7F]).r(), _64b | sse2),
+        inst("movdqu", fmt("A", [w(xmm1), r(xmm_m128)]), rex([0xF3, 0x0F, 0x6F]).r(), _64b | sse2),
+        inst("movdqu", fmt("B", [w(xmm_m128), r(xmm1)]), rex([0xF3, 0x0F, 0x7F]).r(), _64b | sse2),
+    ]
+}

cranelift/assembler-x64/src/api.rs

@@ -252,4 +252,12 @@ pub trait RegisterVisitor<R: Registers> {
             XmmMem::Mem(m) => self.read_amode(m),
         }
     }
+
+    /// Helper method to handle a write [`XmmMem`] operand.
+    fn write_xmm_mem(&mut self, op: &mut XmmMem<R::WriteXmm, R::ReadGpr>) {
+        match op {
+            XmmMem::Xmm(r) => self.write_xmm(r),
+            XmmMem::Mem(m) => self.read_amode(m),
+        }
+    }
 }

cranelift/codegen/meta/src/gen_asm.rs

@@ -25,7 +25,7 @@ pub fn rust_param_raw(op: &Operand) -> String {
             format!("&{reg}Mem{aligned}")
         }
         OperandKind::Mem(_) => {
-            format!("&Amode")
+            format!("&SyntheticAmode")
         }
         OperandKind::Reg(r) | OperandKind::FixedReg(r) => r.reg_class().unwrap().to_string(),
     }
@@ -223,9 +223,9 @@ pub fn isle_param_raw(op: &Operand) -> String {
         OperandKind::Reg(r) | OperandKind::FixedReg(r) => r.reg_class().unwrap().to_string(),
         OperandKind::Mem(_) => {
             if op.align {
-                unimplemented!("no way yet to mark an Amode as aligned")
+                unimplemented!("no way yet to mark an SyntheticAmode as aligned")
             } else {
-                "Amode".to_string()
+                "SyntheticAmode".to_string()
             }
         }
         OperandKind::RegMem(rm) => {
@@ -316,7 +316,7 @@ pub fn isle_param_for_ctor(op: &Operand, ctor: IsleConstructor) -> String {
         // other constructor it's operating on registers so the argument is
         // a `Gpr`.
         OperandKind::RegMem(_) if op.mutability.is_write() => match ctor {
-            IsleConstructor::RetMemorySideEffect => "Amode".to_string(),
+            IsleConstructor::RetMemorySideEffect => "SyntheticAmode".to_string(),
             IsleConstructor::RetGpr => "Gpr".to_string(),
             IsleConstructor::RetXmm => "Xmm".to_string(),
             IsleConstructor::RetValueRegs => "ValueRegs".to_string(),
@@ -581,5 +581,9 @@ pub fn generate_isle(f: &mut Formatter, insts: &[Inst]) {
 /// `RegMem::Mem`, an operand from the constructor of the original entrypoint
 /// itself.
 fn is_raw_operand_param(o: &Operand) -> bool {
-    o.mutability.is_read() || matches!(o.location.kind(), OperandKind::RegMem(_))
+    o.mutability.is_read()
+        || matches!(
+            o.location.kind(),
+            OperandKind::RegMem(_) | OperandKind::Mem(_)
+        )
 }

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

@@ -330,12 +330,6 @@
                         (src XmmMem)
                         (dst WritableXmm))
 
-       ;; XMM (scalar or vector) unary op (from xmm to reg/mem): stores, movd,
-       ;; movq
-       (XmmMovRM (op SseOpcode)
-                 (src Xmm)
-                 (dst SyntheticAmode))
-
        ;; Conversion from signed integers to floats, the `{v,}`cvtsi2s{s,d}`
        ;; instructions.
        ;;
@@ -764,15 +758,7 @@
             Divss
             Divsd
             Insertps
-            Movaps
-            Movapd
-            Movdqa
-            Movdqu
             Movlhps
-            Movss
-            Movsd
-            Movups
-            Movupd
             Pabsb
             Pabsw
             Pabsd
@@ -2285,57 +2271,49 @@
         dst))
 
 (decl x64_movss_load (SyntheticAmode) Xmm)
-(rule (x64_movss_load from)
-      (xmm_unary_rm_r_unaligned (SseOpcode.Movss) from))
+(rule (x64_movss_load from) (x64_movss_a_m from))
 (rule 1 (x64_movss_load from)
         (if-let true (use_avx))
         (xmm_unary_rm_r_vex (AvxOpcode.Vmovss) from))
 
 (decl x64_movss_store (SyntheticAmode Xmm) SideEffectNoResult)
-(rule (x64_movss_store addr data)
-      (xmm_movrm (SseOpcode.Movss) addr data))
+(rule (x64_movss_store addr data) (x64_movss_c_m_mem addr data))
 (rule 1 (x64_movss_store addr data)
         (if-let true (use_avx))
         (xmm_movrm_vex (AvxOpcode.Vmovss) addr data))
 
 (decl x64_movsd_load (SyntheticAmode) Xmm)
-(rule (x64_movsd_load from)
-      (xmm_unary_rm_r_unaligned (SseOpcode.Movsd) from))
+(rule (x64_movsd_load from) (x64_movsd_a_m from))
 (rule 1 (x64_movsd_load from)
         (if-let true (use_avx))
         (xmm_unary_rm_r_vex (AvxOpcode.Vmovsd) from))
 
 (decl x64_movsd_store (SyntheticAmode Xmm) SideEffectNoResult)
-(rule (x64_movsd_store addr data)
-      (xmm_movrm (SseOpcode.Movsd) addr data))
+(rule (x64_movsd_store addr data) (x64_movsd_c_m_mem addr data))
 (rule 1 (x64_movsd_store addr data)
         (if-let true (use_avx))
         (xmm_movrm_vex (AvxOpcode.Vmovsd) addr data))
 
 (decl x64_movups_load (SyntheticAmode) Xmm)
-(rule (x64_movups_load from)
-      (xmm_unary_rm_r_unaligned (SseOpcode.Movups) from))
+(rule (x64_movups_load from) (x64_movups_a from))
 (rule 1 (x64_movups_load from)
         (if-let true (use_avx))
         (xmm_unary_rm_r_vex (AvxOpcode.Vmovups) from))
 
 (decl x64_movups_store (SyntheticAmode Xmm) SideEffectNoResult)
-(rule (x64_movups_store addr data)
-      (xmm_movrm (SseOpcode.Movups) addr data))
+(rule (x64_movups_store addr data) (x64_movups_b_mem addr data))
 (rule 1 (x64_movups_store addr data)
         (if-let true (use_avx))
         (xmm_movrm_vex (AvxOpcode.Vmovups) addr data))
 
 (decl x64_movupd_load (SyntheticAmode) Xmm)
-(rule (x64_movupd_load from)
-      (xmm_unary_rm_r_unaligned (SseOpcode.Movupd) from))
+(rule (x64_movupd_load from) (x64_movupd_a from))
 (rule 1 (x64_movupd_load from)
         (if-let true (use_avx))
         (xmm_unary_rm_r_vex (AvxOpcode.Vmovupd) from))
 
 (decl x64_movupd_store (SyntheticAmode Xmm) SideEffectNoResult)
-(rule (x64_movupd_store addr data)
-      (xmm_movrm (SseOpcode.Movupd) addr data))
+(rule (x64_movupd_store addr data) (x64_movupd_b_mem addr data))
 (rule 1 (x64_movupd_store addr data)
         (if-let true (use_avx))
         (xmm_movrm_vex (AvxOpcode.Vmovupd) addr data))
@@ -2369,15 +2347,13 @@
         (xmm_to_gpr_vex (AvxOpcode.Vmovq) from (OperandSize.Size64)))
 
 (decl x64_movdqu_load (XmmMem) Xmm)
-(rule (x64_movdqu_load from)
-      (xmm_unary_rm_r_unaligned (SseOpcode.Movdqu) from))
+(rule (x64_movdqu_load from) (x64_movdqu_a from))
 (rule 1 (x64_movdqu_load from)
         (if-let true (use_avx))
         (xmm_unary_rm_r_vex (AvxOpcode.Vmovdqu) from))
 
 (decl x64_movdqu_store (SyntheticAmode Xmm) SideEffectNoResult)
-(rule (x64_movdqu_store addr data)
-      (xmm_movrm (SseOpcode.Movdqu) addr data))
+(rule (x64_movdqu_store addr data) (x64_movdqu_b_mem addr data))
 (rule 1 (x64_movdqu_store addr data)
         (if-let true (use_avx))
         (xmm_movrm_vex (AvxOpcode.Vmovdqu) addr data))
@@ -2436,10 +2412,6 @@
       (let ((size OperandSize (raw_operand_size_of_type ty)))
         (SideEffectNoResult.Inst (MInst.MovImmM size imm addr))))
 
-(decl xmm_movrm (SseOpcode SyntheticAmode Xmm) SideEffectNoResult)
-(rule (xmm_movrm op addr data)
-      (SideEffectNoResult.Inst (MInst.XmmMovRM op data addr)))
-
 (decl xmm_movrm_vex (AvxOpcode SyntheticAmode Xmm) SideEffectNoResult)
 (rule (xmm_movrm_vex op addr data)
       (SideEffectNoResult.Inst (MInst.XmmMovRMVex op data addr)))
@@ -3681,15 +3653,13 @@
 ;; when used as a load instruction it wipes out the entire destination register
 ;; which defeats the purpose of this being a 2-operand instruction.
 (decl x64_movsd_regmove (Xmm Xmm) Xmm)
-(rule (x64_movsd_regmove src1 src2)
-      (xmm_rm_r_unaligned (SseOpcode.Movsd) src1 src2))
+(rule (x64_movsd_regmove src1 src2) (x64_movsd_a_r src1 src2))
 (rule 1 (x64_movsd_regmove src1 src2)
         (if-let true (use_avx))
         (xmm_rmir_vex (AvxOpcode.Vmovsd) src1 src2))
 
 (decl x64_movss_regmove (Xmm Xmm) Xmm)
-(rule (x64_movss_regmove src1 src2)
-      (xmm_rm_r_unaligned (SseOpcode.Movss) src1 src2))
+(rule (x64_movss_regmove src1 src2) (x64_movss_a_r src1 src2))
 (rule 1 (x64_movss_regmove src1 src2)
         (if-let true (use_avx))
         (xmm_rmir_vex (AvxOpcode.Vmovss) src1 src2))
@@ -5438,6 +5408,7 @@
 (convert WritableXmm WritableReg writable_xmm_to_reg)
 (convert WritableXmm Reg writable_xmm_to_r_reg)
 (convert WritableXmm XmmMem writable_xmm_to_xmm_mem)
+(convert WritableXmm XmmMemAligned writable_xmm_to_xmm_mem_aligned)
 (convert WritableXmm ValueRegs writable_xmm_to_value_regs)
 
 ;; Note that these conversions will introduce a `movupd` instruction if
@@ -5496,6 +5467,9 @@
 (decl writable_xmm_to_xmm_mem (WritableXmm) XmmMem)
 (rule (writable_xmm_to_xmm_mem w_xmm)
       (xmm_to_xmm_mem (writable_xmm_to_xmm w_xmm)))
+(decl writable_xmm_to_xmm_mem_aligned (WritableXmm) XmmMemAligned)
+(rule (writable_xmm_to_xmm_mem_aligned w_xmm)
+      (xmm_to_xmm_mem_aligned (writable_xmm_to_xmm w_xmm)))
 (decl writable_xmm_to_value_regs (WritableXmm) ValueRegs)
 (rule (writable_xmm_to_value_regs w_xmm)
       (value_reg w_xmm))

cranelift/codegen/src/isa/x64/inst/args.rs

@@ -834,15 +834,7 @@ pub enum SseOpcode {
     Divss,
     Divsd,
     Insertps,
-    Movaps,
-    Movapd,
-    Movdqa,
-    Movdqu,
     Movlhps,
-    Movss,
-    Movsd,
-    Movups,
-    Movupd,
     Pabsb,
     Pabsw,
     Pabsd,
@@ -911,10 +903,7 @@ impl SseOpcode {
             | SseOpcode::Cmpss
             | SseOpcode::Divps
             | SseOpcode::Divss
-            | SseOpcode::Movaps
             | SseOpcode::Movlhps
-            | SseOpcode::Movss
-            | SseOpcode::Movups
             | SseOpcode::Rcpss
             | SseOpcode::Rsqrtss
             | SseOpcode::Shufps
@@ -925,11 +914,6 @@ impl SseOpcode {
             | SseOpcode::Comisd
             | SseOpcode::Divpd
             | SseOpcode::Divsd
-            | SseOpcode::Movapd
-            | SseOpcode::Movsd
-            | SseOpcode::Movupd
-            | SseOpcode::Movdqa
-            | SseOpcode::Movdqu
             | SseOpcode::Packssdw
             | SseOpcode::Packsswb
             | SseOpcode::Packuswb
@@ -1034,15 +1018,7 @@ impl fmt::Debug for SseOpcode {
             SseOpcode::Divss => "divss",
             SseOpcode::Divsd => "divsd",
             SseOpcode::Insertps => "insertps",
-            SseOpcode::Movaps => "movaps",
-            SseOpcode::Movapd => "movapd",
-            SseOpcode::Movdqa => "movdqa",
-            SseOpcode::Movdqu => "movdqu",
             SseOpcode::Movlhps => "movlhps",
-            SseOpcode::Movss => "movss",
-            SseOpcode::Movsd => "movsd",
-            SseOpcode::Movups => "movups",
-            SseOpcode::Movupd => "movupd",
             SseOpcode::Pabsb => "pabsb",
             SseOpcode::Pabsw => "pabsw",
             SseOpcode::Pabsd => "pabsd",