Refactor the InstSize enum in the AArch64 backend

The main issue with the InstSize enum was that it was used both for
GPR and SIMD & FP operands, even though machine instructions do not
mix them in general (as in a destination register is either a GPR
or not). As a result it had methods such as sf_bit() that made
sense only for one type of operand.

Another issue was that the enum name was not reflecting its purpose
accurately - it was meant to represent an instruction operand size,
not an instruction size, which is fixed in A64 (always 4 bytes).

Now the enum is split into one for GPR operands and another for
scalar SIMD & FP operands.

Copyright (c) 2020, Arm Limited.

Author: akirilov-arm

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

@@ -403,8 +403,8 @@ impl ShowWithRRU for MemArg {
             &MemArg::RegScaledExtended(r1, r2, ty, op) => {
                 let shift = shift_for_type(ty);
                 let size = match op {
-                    ExtendOp::SXTW | ExtendOp::UXTW => InstSize::Size32,
-                    _ => InstSize::Size64,
+                    ExtendOp::SXTW | ExtendOp::UXTW => OperandSize::Size32,
+                    _ => OperandSize::Size64,
                 };
                 let op = op.show_rru(mb_rru);
                 format!(
@@ -417,8 +417,8 @@ impl ShowWithRRU for MemArg {
             }
             &MemArg::RegExtended(r1, r2, op) => {
                 let size = match op {
-                    ExtendOp::SXTW | ExtendOp::UXTW => InstSize::Size32,
-                    _ => InstSize::Size64,
+                    ExtendOp::SXTW | ExtendOp::UXTW => OperandSize::Size32,
+                    _ => OperandSize::Size64,
                 };
                 let op = op.show_rru(mb_rru);
                 format!(
@@ -492,67 +492,98 @@ impl ShowWithRRU for BranchTarget {
 }
 
 /// Type used to communicate the operand size of a machine instruction, as AArch64 has 32- and
-/// 64-bit variants of many instructions (and integer and floating-point registers) and 128-bit
-/// variants of vector instructions.
-/// TODO: Create a separate type for SIMD & floating-point operands.
+/// 64-bit variants of many instructions (and integer registers).
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
-pub enum InstSize {
+pub enum OperandSize {
     Size32,
     Size64,
-    Size128,
 }
 
-impl InstSize {
+impl OperandSize {
     /// 32-bit case?
     pub fn is32(self) -> bool {
-        self == InstSize::Size32
+        self == OperandSize::Size32
     }
     /// 64-bit case?
     pub fn is64(self) -> bool {
-        self == InstSize::Size64
+        self == OperandSize::Size64
     }
-    /// Convert from an `is32` boolean flag to an `InstSize`.
-    pub fn from_is32(is32: bool) -> InstSize {
+    /// Convert from an `is32` boolean flag to an `OperandSize`.
+    pub fn from_is32(is32: bool) -> OperandSize {
         if is32 {
-            InstSize::Size32
+            OperandSize::Size32
         } else {
-            InstSize::Size64
+            OperandSize::Size64
         }
     }
     /// Convert from a needed width to the smallest size that fits.
-    pub fn from_bits<I: Into<usize>>(bits: I) -> InstSize {
+    pub fn from_bits<I: Into<usize>>(bits: I) -> OperandSize {
         let bits: usize = bits.into();
-        assert!(bits <= 128);
+        assert!(bits <= 64);
         if bits <= 32 {
-            InstSize::Size32
-        } else if bits <= 64 {
-            InstSize::Size64
+            OperandSize::Size32
         } else {
-            InstSize::Size128
+            OperandSize::Size64
         }
     }
 
     /// Convert from an integer type into the smallest size that fits.
-    pub fn from_ty(ty: Type) -> InstSize {
+    pub fn from_ty(ty: Type) -> OperandSize {
         Self::from_bits(ty_bits(ty))
     }
 
     /// Convert to I32, I64, or I128.
     pub fn to_ty(self) -> Type {
         match self {
-            InstSize::Size32 => I32,
-            InstSize::Size64 => I64,
-            InstSize::Size128 => I128,
+            OperandSize::Size32 => I32,
+            OperandSize::Size64 => I64,
         }
     }
 
     pub fn sf_bit(&self) -> u32 {
         match self {
-            InstSize::Size32 => 0,
-            InstSize::Size64 => 1,
-            _ => {
-                panic!("Unexpected size");
-            }
+            OperandSize::Size32 => 0,
+            OperandSize::Size64 => 1,
+        }
+    }
+}
+
+/// Type used to communicate the size of a scalar SIMD & FP operand.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum ScalarSize {
+    Size8,
+    Size16,
+    Size32,
+    Size64,
+    Size128,
+}
+
+impl ScalarSize {
+    /// Convert from a needed width to the smallest size that fits.
+    pub fn from_bits<I: Into<usize>>(bits: I) -> ScalarSize {
+        match bits.into().next_power_of_two() {
+            8 => ScalarSize::Size8,
+            16 => ScalarSize::Size16,
+            32 => ScalarSize::Size32,
+            64 => ScalarSize::Size64,
+            128 => ScalarSize::Size128,
+            _ => panic!("Unexpected type width"),
+        }
+    }
+
+    /// Convert from a type into the smallest size that fits.
+    pub fn from_ty(ty: Type) -> ScalarSize {
+        Self::from_bits(ty_bits(ty))
+    }
+
+    /// Return the encoding bits that are used by some scalar FP instructions
+    /// for a particular operand size.
+    pub fn ftype(&self) -> u32 {
+        match self {
+            ScalarSize::Size16 => 0b11,
+            ScalarSize::Size32 => 0b00,
+            ScalarSize::Size64 => 0b01,
+            _ => panic!("Unexpected scalar FP operand size"),
         }
     }
 }

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

@@ -282,14 +282,13 @@ fn enc_csel(rd: Writable<Reg>, rn: Reg, rm: Reg, cond: Cond) -> u32 {
         | (cond.bits() << 12)
 }
 
-fn enc_fcsel(rd: Writable<Reg>, rn: Reg, rm: Reg, cond: Cond, size: InstSize) -> u32 {
-    let ty_bit = if size.is32() { 0 } else { 1 };
+fn enc_fcsel(rd: Writable<Reg>, rn: Reg, rm: Reg, cond: Cond, size: ScalarSize) -> u32 {
     0b000_11110_00_1_00000_0000_11_00000_00000
+        | (size.ftype() << 22)
         | (machreg_to_vec(rm) << 16)
         | (machreg_to_vec(rn) << 5)
         | machreg_to_vec(rd.to_reg())
         | (cond.bits() << 12)
-        | (ty_bit << 22)
 }
 
 fn enc_cset(rd: Writable<Reg>, cond: Cond) -> u32 {
@@ -298,7 +297,7 @@ fn enc_cset(rd: Writable<Reg>, cond: Cond) -> u32 {
         | (cond.invert().bits() << 12)
 }
 
-fn enc_ccmp_imm(size: InstSize, rn: Reg, imm: UImm5, nzcv: NZCV, cond: Cond) -> u32 {
+fn enc_ccmp_imm(size: OperandSize, rn: Reg, imm: UImm5, nzcv: NZCV, cond: Cond) -> u32 {
     0b0_1_1_11010010_00000_0000_10_00000_0_0000
         | size.sf_bit() << 31
         | imm.bits() << 16
@@ -334,13 +333,11 @@ fn enc_fpurrrr(top17: u32, rd: Writable<Reg>, rn: Reg, rm: Reg, ra: Reg) -> u32
         | machreg_to_vec(rd.to_reg())
 }
 
-fn enc_fcmp(size: InstSize, rn: Reg, rm: Reg) -> u32 {
-    let bits = if size.is32() {
-        0b000_11110_00_1_00000_00_1000_00000_00000
-    } else {
-        0b000_11110_01_1_00000_00_1000_00000_00000
-    };
-    bits | (machreg_to_vec(rm) << 16) | (machreg_to_vec(rn) << 5)
+fn enc_fcmp(size: ScalarSize, rn: Reg, rm: Reg) -> u32 {
+    0b000_11110_00_1_00000_00_1000_00000_00000
+        | (size.ftype() << 22)
+        | (machreg_to_vec(rm) << 16)
+        | (machreg_to_vec(rn) << 5)
 }
 
 fn enc_fputoint(top16: u32, rd: Writable<Reg>, rn: Reg) -> u32 {
@@ -613,7 +610,7 @@ impl MachInstEmit for Inst {
             }
 
             &Inst::BitRR { op, rd, rn, .. } => {
-                let size = if op.inst_size().is32() { 0b0 } else { 0b1 };
+                let size = if op.operand_size().is32() { 0b0 } else { 0b1 };
                 let (op1, op2) = match op {
                     BitOp::RBit32 | BitOp::RBit64 => (0b00000, 0b000000),
                     BitOp::Clz32 | BitOp::Clz64 => (0b00000, 0b000100),
@@ -979,10 +976,10 @@ impl MachInstEmit for Inst {
             &Inst::FpuMove128 { rd, rn } => {
                 sink.put4(enc_vecmov(/* 16b = */ true, rd, rn));
             }
-            &Inst::FpuMoveFromVec { rd, rn, idx, ty } => {
-                let (imm5, shift, mask) = match ty {
-                    F32 => (0b00100, 3, 0b011),
-                    F64 => (0b01000, 4, 0b001),
+            &Inst::FpuMoveFromVec { rd, rn, idx, size } => {
+                let (imm5, shift, mask) = match size {
+                    ScalarSize::Size32 => (0b00100, 3, 0b011),
+                    ScalarSize::Size64 => (0b01000, 4, 0b001),
                     _ => unimplemented!(),
                 };
                 debug_assert_eq!(idx & mask, idx);
@@ -1108,10 +1105,10 @@ impl MachInstEmit for Inst {
                 sink.put4(enc_vec_lanes(q, u, size, opcode, rd, rn));
             }
             &Inst::FpuCmp32 { rn, rm } => {
-                sink.put4(enc_fcmp(InstSize::Size32, rn, rm));
+                sink.put4(enc_fcmp(ScalarSize::Size32, rn, rm));
             }
             &Inst::FpuCmp64 { rn, rm } => {
-                sink.put4(enc_fcmp(InstSize::Size64, rn, rm));
+                sink.put4(enc_fcmp(ScalarSize::Size64, rn, rm));
             }
             &Inst::FpuToInt { op, rd, rn } => {
                 let top16 = match op {
@@ -1198,10 +1195,10 @@ impl MachInstEmit for Inst {
                 }
             }
             &Inst::FpuCSel32 { rd, rn, rm, cond } => {
-                sink.put4(enc_fcsel(rd, rn, rm, cond, InstSize::Size32));
+                sink.put4(enc_fcsel(rd, rn, rm, cond, ScalarSize::Size32));
             }
             &Inst::FpuCSel64 { rd, rn, rm, cond } => {
-                sink.put4(enc_fcsel(rd, rn, rm, cond, InstSize::Size64));
+                sink.put4(enc_fcsel(rd, rn, rm, cond, ScalarSize::Size64));
             }
             &Inst::FpuRound { op, rd, rn } => {
                 let top22 = match op {

cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs

@@ -1808,7 +1808,7 @@ fn test_aarch64_binemit() {
     ));
     insns.push((
         Inst::CCmpImm {
-            size: InstSize::Size64,
+            size: OperandSize::Size64,
             rn: xreg(22),
             imm: UImm5::maybe_from_u8(5).unwrap(),
             nzcv: NZCV::new(false, false, true, true),
@@ -1819,7 +1819,7 @@ fn test_aarch64_binemit() {
     ));
     insns.push((
         Inst::CCmpImm {
-            size: InstSize::Size32,
+            size: OperandSize::Size32,
             rn: xreg(3),
             imm: UImm5::maybe_from_u8(30).unwrap(),
             nzcv: NZCV::new(true, true, true, true),
@@ -3022,7 +3022,7 @@ fn test_aarch64_binemit() {
             rd: writable_vreg(1),
             rn: vreg(30),
             idx: 2,
-            ty: F32,
+            size: ScalarSize::Size32,
         },
         "C107145E",
         "mov s1, v30.s[2]",
@@ -3033,7 +3033,7 @@ fn test_aarch64_binemit() {
             rd: writable_vreg(23),
             rn: vreg(11),
             idx: 0,
-            ty: F64,
+            size: ScalarSize::Size64,
         },
         "7705085E",
         "mov d23, v11.d[0]",