x64: Remove conditional SseOpcode::uses_src1 (#5842)

This is a follow-up to comments in #5795 to remove some cruft in the x64
instruction model to ensure that the shape of an `Inst` reflects what's
going to happen in regalloc and encoding. This accessor was used to
handle `round*`, `pextr*`, and `pshufb` instructions. The `round*` ones
had already moved to the appropriate `XmmUnary*` variant and `pshufb`
was additionally moved over to that variant as well.

The `pextr*` instructions got a new `Inst` variant and additionally had
their constructors slightly modified to no longer require the type as
input. The encoding for these instructions now automatically handles the
various type-related operands through a new `SseOpcode::Pextrq` operand
to represent 64-bit movements.

Author: alexcrichton

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

@@ -331,6 +331,12 @@
                  (dst WritableGpr)
                  (dst_size OperandSize))
 
+       ;; XMM (scalar) unary op (from xmm to integer reg): pextr{w,b,d,q}
+       (XmmToGprImm (op SseOpcode)
+                    (src Xmm)
+                    (dst WritableGpr)
+                    (imm u8))
+
        ;; XMM (scalar) unary op (from integer to float reg): movd, movq,
        ;; cvtsi2s{s,d}
        (GprToXmm (op SseOpcode)
@@ -749,6 +755,7 @@
             Pextrb
             Pextrw
             Pextrd
+            Pextrq
             Pinsrb
             Pinsrw
             Pinsrd
@@ -3110,16 +3117,9 @@
       (xmm_rmr_imm_vex (AvxOpcode.Vinsertps) src1 src2 lane))
 
 ;; Helper for creating `pshufd` instructions.
-(decl x64_pshufd (XmmMem u8 OperandSize) Xmm)
-(rule (x64_pshufd src imm size)
-      (let ((dst WritableXmm (temp_writable_xmm))
-            (_ Unit (emit (MInst.XmmRmRImm (SseOpcode.Pshufd)
-                                           dst
-                                           src
-                                           dst
-                                           imm
-                                           size))))
-        dst))
+(decl x64_pshufd (XmmMem u8) Xmm)
+(rule (x64_pshufd src imm)
+      (xmm_unary_rm_r_imm (SseOpcode.Pshufd) src imm))
 
 ;; Helper for creating `pshufb` instructions.
 (decl x64_pshufb (Xmm XmmMem) Xmm)
@@ -3314,40 +3314,24 @@
       (xmm_rmir_vex (AvxOpcode.Vpsrad) src1 src2))
 
 ;; Helper for creating `pextrb` instructions.
-(decl x64_pextrb (Type Xmm u8) Gpr)
-(rule (x64_pextrb ty src lane)
-      (let ((dst WritableGpr (temp_writable_gpr))
-            (_ Unit (emit (MInst.XmmRmRImm (SseOpcode.Pextrb)
-                                           dst
-                                           src
-                                           dst
-                                           lane
-                                           (operand_size_of_type_32_64 (lane_type ty))))))
-        dst))
+(decl x64_pextrb (Xmm u8) Gpr)
+(rule (x64_pextrb src lane)
+      (xmm_to_gpr_imm (SseOpcode.Pextrb) src lane))
 
 ;; Helper for creating `pextrw` instructions.
-(decl x64_pextrw (Type Xmm u8) Gpr)
-(rule (x64_pextrw ty src lane)
-      (let ((dst WritableGpr (temp_writable_gpr))
-            (_ Unit (emit (MInst.XmmRmRImm (SseOpcode.Pextrw)
-                                           dst
-                                           src
-                                           dst
-                                           lane
-                                           (operand_size_of_type_32_64 (lane_type ty))))))
-        dst))
+(decl x64_pextrw (Xmm u8) Gpr)
+(rule (x64_pextrw src lane)
+      (xmm_to_gpr_imm (SseOpcode.Pextrw) src lane))
 
 ;; Helper for creating `pextrd` instructions.
-(decl x64_pextrd (Type Xmm u8) Gpr)
-(rule (x64_pextrd ty src lane)
-      (let ((dst WritableGpr (temp_writable_gpr))
-            (_ Unit (emit (MInst.XmmRmRImm (SseOpcode.Pextrd)
-                                           dst
-                                           src
-                                           dst
-                                           lane
-                                           (operand_size_of_type_32_64 (lane_type ty))))))
-        dst))
+(decl x64_pextrd (Xmm u8) Gpr)
+(rule (x64_pextrd src lane)
+      (xmm_to_gpr_imm (SseOpcode.Pextrd) src lane))
+
+;; Helper for creating `pextrq` instructions.
+(decl x64_pextrq (Xmm u8) Gpr)
+(rule (x64_pextrq src lane)
+      (xmm_to_gpr_imm (SseOpcode.Pextrq) src lane))
 
 ;; Helper for creating `MInst.XmmToGpr` instructions.
 (decl xmm_to_gpr (SseOpcode Xmm OperandSize) Gpr)
@@ -3356,6 +3340,13 @@
             (_ Unit (emit (MInst.XmmToGpr op src dst size))))
         dst))
 
+;; Helper for creating `MInst.XmmToGpr` instructions.
+(decl xmm_to_gpr_imm (SseOpcode Xmm u8) Gpr)
+(rule (xmm_to_gpr_imm op src imm)
+      (let ((dst WritableGpr (temp_writable_gpr))
+            (_ Unit (emit (MInst.XmmToGprImm op src dst imm))))
+        dst))
+
 ;; Helper for creating `pmovmskb` instructions.
 (decl x64_pmovmskb (OperandSize Xmm) Gpr)
 (rule (x64_pmovmskb size src)

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

@@ -999,6 +999,7 @@ pub enum SseOpcode {
     Pextrb,
     Pextrw,
     Pextrd,
+    Pextrq,
     Pinsrb,
     Pinsrw,
     Pinsrd,
@@ -1237,6 +1238,7 @@ impl SseOpcode {
             | SseOpcode::Pcmpeqq
             | SseOpcode::Pextrb
             | SseOpcode::Pextrd
+            | SseOpcode::Pextrq
             | SseOpcode::Pinsrb
             | SseOpcode::Pinsrd
             | SseOpcode::Pmaxsb
@@ -1278,22 +1280,6 @@ impl SseOpcode {
             _ => 8,
         }
     }
-
-    /// Does an XmmRmmRImm with this opcode use src1? FIXME: split
-    /// into separate instructions.
-    pub(crate) fn uses_src1(&self) -> bool {
-        match self {
-            SseOpcode::Pextrb => false,
-            SseOpcode::Pextrw => false,
-            SseOpcode::Pextrd => false,
-            SseOpcode::Pshufd => false,
-            SseOpcode::Roundss => false,
-            SseOpcode::Roundsd => false,
-            SseOpcode::Roundps => false,
-            SseOpcode::Roundpd => false,
-            _ => true,
-        }
-    }
 }
 
 impl fmt::Debug for SseOpcode {
@@ -1393,6 +1379,7 @@ impl fmt::Debug for SseOpcode {
             SseOpcode::Pextrb => "pextrb",
             SseOpcode::Pextrw => "pextrw",
             SseOpcode::Pextrd => "pextrd",
+            SseOpcode::Pextrq => "pextrq",
             SseOpcode::Pinsrb => "pinsrb",
             SseOpcode::Pinsrw => "pinsrw",
             SseOpcode::Pinsrd => "pinsrd",

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

@@ -1792,8 +1792,6 @@ pub(crate) fn emit(
         }
 
         Inst::XmmUnaryRmRImm { op, src, dst, imm } => {
-            debug_assert!(!op.uses_src1());
-
             let dst = allocs.next(dst.to_reg().to_reg());
             let src = src.clone().to_reg_mem().with_allocs(allocs);
             let rex = RexFlags::clear_w();
@@ -1803,6 +1801,7 @@ pub(crate) fn emit(
                 SseOpcode::Roundss => (LegacyPrefixes::_66, 0x0F3A0A, 3),
                 SseOpcode::Roundpd => (LegacyPrefixes::_66, 0x0F3A09, 3),
                 SseOpcode::Roundsd => (LegacyPrefixes::_66, 0x0F3A0B, 3),
+                SseOpcode::Pshufd => (LegacyPrefixes::_66, 0x0F70, 2),
                 _ => unimplemented!("Opcode {:?} not implemented", op),
             };
             match src {
@@ -2458,17 +2457,10 @@ pub(crate) fn emit(
             imm,
             size,
         } => {
-            let (src2, dst) = if !op.uses_src1() {
-                let dst = allocs.next(dst.to_reg());
-                let src2 = src2.with_allocs(allocs);
-                (src2, dst)
-            } else {
-                let src1 = allocs.next(*src1);
-                let dst = allocs.next(dst.to_reg());
-                let src2 = src2.with_allocs(allocs);
-                debug_assert_eq!(src1, dst);
-                (src2, dst)
-            };
+            let src1 = allocs.next(*src1);
+            let dst = allocs.next(dst.to_reg());
+            let src2 = src2.with_allocs(allocs);
+            debug_assert_eq!(src1, dst);
 
             let (prefix, opcode, len) = match op {
                 SseOpcode::Cmpps => (LegacyPrefixes::None, 0x0FC2, 2),
@@ -2480,10 +2472,6 @@ pub(crate) fn emit(
                 SseOpcode::Pinsrb => (LegacyPrefixes::_66, 0x0F3A20, 3),
                 SseOpcode::Pinsrw => (LegacyPrefixes::_66, 0x0FC4, 2),
                 SseOpcode::Pinsrd => (LegacyPrefixes::_66, 0x0F3A22, 3),
-                SseOpcode::Pextrb => (LegacyPrefixes::_66, 0x0F3A14, 3),
-                SseOpcode::Pextrw => (LegacyPrefixes::_66, 0x0FC5, 2),
-                SseOpcode::Pextrd => (LegacyPrefixes::_66, 0x0F3A16, 3),
-                SseOpcode::Pshufd => (LegacyPrefixes::_66, 0x0F70, 2),
                 SseOpcode::Shufps => (LegacyPrefixes::None, 0x0FC6, 2),
                 _ => unimplemented!("Opcode {:?} not implemented", op),
             };
@@ -2566,6 +2554,26 @@ pub(crate) fn emit(
             emit_std_reg_reg(sink, prefix, opcode, 2, src, dst, rex);
         }
 
+        Inst::XmmToGprImm { op, src, dst, imm } => {
+            use OperandSize as OS;
+
+            let src = allocs.next(src.to_reg());
+            let dst = allocs.next(dst.to_reg().to_reg());
+
+            let (prefix, opcode, opcode_bytes, dst_size, dst_first) = match op {
+                SseOpcode::Pextrb => (LegacyPrefixes::_66, 0x0F3A14, 3, OS::Size32, false),
+                SseOpcode::Pextrw => (LegacyPrefixes::_66, 0x0FC5, 2, OS::Size32, true),
+                SseOpcode::Pextrd => (LegacyPrefixes::_66, 0x0F3A16, 3, OS::Size32, false),
+                SseOpcode::Pextrq => (LegacyPrefixes::_66, 0x0F3A16, 3, OS::Size64, false),
+                _ => panic!("unexpected opcode {:?}", op),
+            };
+            let rex = RexFlags::from(dst_size);
+            let (src, dst) = if dst_first { (dst, src) } else { (src, dst) };
+
+            emit_std_reg_reg(sink, prefix, opcode, opcode_bytes, src, dst, rex);
+            sink.put1(*imm);
+        }
+
         Inst::GprToXmm {
             op,
             src: src_e,

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

@@ -136,6 +136,7 @@ impl Inst {
             | Inst::XmmRmRBlend { op, .. }
             | Inst::XmmRmRImm { op, .. }
             | Inst::XmmToGpr { op, .. }
+            | Inst::XmmToGprImm { op, .. }
             | Inst::XmmUnaryRmRImm { op, .. }
             | Inst::XmmUnaryRmR { op, .. }
             | Inst::XmmConstOp { op, .. } => smallvec![op.available_from()],
@@ -1111,15 +1112,11 @@ impl PrettyPrint for Inst {
                 size,
                 ..
             } => {
-                let src1 = if op.uses_src1() {
-                    pretty_print_reg(*src1, 8, allocs) + ", "
-                } else {
-                    "".into()
-                };
+                let src1 = pretty_print_reg(*src1, 8, allocs);
                 let dst = pretty_print_reg(dst.to_reg(), 8, allocs);
                 let src2 = src2.pretty_print(8, allocs);
                 format!(
-                    "{} ${}, {}{}, {}",
+                    "{} ${imm}, {src1}, {src2}, {dst}",
                     ljustify(format!(
                         "{}{}",
                         op.to_string(),
@@ -1129,10 +1126,6 @@ impl PrettyPrint for Inst {
                             ""
                         }
                     )),
-                    imm,
-                    src1,
-                    src2,
-                    dst,
                 )
             }
 
@@ -1153,6 +1146,12 @@ impl PrettyPrint for Inst {
                 format!("{} {}, {}", ljustify(op.to_string()), src, dst)
             }
 
+            Inst::XmmToGprImm { op, src, dst, imm } => {
+                let src = pretty_print_reg(src.to_reg(), 8, allocs);
+                let dst = pretty_print_reg(dst.to_reg().to_reg(), 8, allocs);
+                format!("{} ${imm}, {}, {}", ljustify(op.to_string()), src, dst)
+            }
+
             Inst::GprToXmm {
                 op,
                 src,
@@ -1976,23 +1975,11 @@ fn x64_get_operands<F: Fn(VReg) -> VReg>(inst: &Inst, collector: &mut OperandCol
             src1.get_operands(collector);
         }
         Inst::XmmRmRImm {
-            op,
-            src1,
-            src2,
-            dst,
-            ..
+            src1, src2, dst, ..
         } => {
-            if !op.uses_src1() {
-                // FIXME: split this instruction into two, so we don't
-                // need this awkward src1-is-only-sometimes-an-arg
-                // behavior.
-                collector.reg_def(*dst);
-                src2.get_operands(collector);
-            } else {
-                collector.reg_use(*src1);
-                collector.reg_reuse_def(*dst, 0);
-                src2.get_operands(collector);
-            }
+            collector.reg_use(*src1);
+            collector.reg_reuse_def(*dst, 0);
+            src2.get_operands(collector);
         }
         Inst::XmmConstOp { dst, .. } => {
             collector.reg_def(dst.to_writable_reg());
@@ -2035,7 +2022,7 @@ fn x64_get_operands<F: Fn(VReg) -> VReg>(inst: &Inst, collector: &mut OperandCol
             collector.reg_use(src.to_reg());
             collector.reg_fixed_nonallocatable(*dst);
         }
-        Inst::XmmToGpr { src, dst, .. } => {
+        Inst::XmmToGpr { src, dst, .. } | Inst::XmmToGprImm { src, dst, .. } => {
             collector.reg_use(src.to_reg());
             collector.reg_def(dst.to_writable_reg());
         }