Rename instructions with immediate values + add ShiftAmount (#1221)

* rename *Rev instruction immediate variants to *Lhs

We will begin to rename immediate variants of binary instructions to tell if `lhs` or `rhs` operand is the immediate by appending either `Rhs` or `Lhs`.

* rename binary div,rem instructions with `rhs` immediates

The new names are now mirroring their `lhs` immediate counterparts and make it more clear where the immediate is positioned.

* add [Into]ShiftAmount definitions to wasmi_ir

* use ShiftAmount in shift and rotate instructions

* rename instructions with immediate ShiftAmount

* remove unnecessary Lhs (_lhs) affix from shift and rotate instrs

* fix intra doc links

crates/ir/src/lib.rs

@@ -27,7 +27,16 @@ pub use self::{
     immeditate::{AnyConst16, AnyConst32, Const16, Const32},
     index::Instr,
     index::Reg,
-    primitive::{BlockFuel, BranchOffset, BranchOffset16, Comparator, ComparatorAndOffset, Sign},
+    primitive::{
+        BlockFuel,
+        BranchOffset,
+        BranchOffset16,
+        Comparator,
+        ComparatorAndOffset,
+        IntoShiftAmount,
+        ShiftAmount,
+        Sign,
+    },
     r#enum::Instruction,
     sequence::{InstrIter, InstrIterMut, InstrSequence},
     span::{BoundedRegSpan, FixedRegSpan, RegSpan, RegSpanIter},

crates/ir/src/primitive.rs

@@ -1,4 +1,4 @@
-use crate::{core::UntypedVal, Error, Instr};
+use crate::{core::UntypedVal, Const16, Error, Instr};
 use core::marker::PhantomData;
 
 /// The sign of a value.
@@ -366,3 +366,51 @@ impl From<ComparatorAndOffset> for UntypedVal {
         Self::from(params.as_u64())
     }
 }
+
+/// A typed shift amount for shift and rotate instructions.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub struct ShiftAmount<T> {
+    /// The underlying wrapped shift amount.
+    value: Const16<T>,
+}
+
+/// Integer ypes that can be used as shift amount in shift or rotate instructions.
+pub trait IntoShiftAmount: Sized {
+    /// Converts `self` into a [`ShiftAmount`] if possible.
+    fn into_shift_amount(self) -> Option<ShiftAmount<Self>>;
+}
+
+macro_rules! impl_shift_amount {
+    ( $( ($ty:ty, $bits:literal) ),* $(,)? ) => {
+        $(
+            impl IntoShiftAmount for $ty {
+                fn into_shift_amount(self) -> Option<ShiftAmount<Self>> {
+                    <ShiftAmount<$ty>>::new(self)
+                }
+            }
+
+            impl ShiftAmount<$ty> {
+                /// Creates a new [`ShiftAmount`] for the given `value`.
+                ///
+                /// Returns `None` if `value` causes a no-op shift.
+                pub fn new(value: $ty) -> Option<Self> {
+                    let value = (value % $bits) as i16;
+                    if value == 0 {
+                        return None
+                    }
+                    Some(Self { value: Const16::from(value) })
+                }
+            }
+
+            impl From<ShiftAmount<$ty>> for $ty {
+                fn from(shamt: ShiftAmount<$ty>) -> Self {
+                    shamt.value.into()
+                }
+            }
+        )*
+    };
+}
+impl_shift_amount! {
+    (i32, 32),
+    (i64, 64),
+}

crates/ir/src/visit_regs.rs

@@ -91,6 +91,7 @@ impl_host_visitor_for!(
     Const16<T>,
     Const32<T>,
     Sign<T>,
+    ShiftAmount<T>,
 );
 
 /// Type-wrapper to signal that the wrapped [`Reg`], [`RegSpan`] (etc.) is a result.

crates/wasmi/src/engine/executor/instrs/binary.rs

@@ -2,6 +2,7 @@ use super::{Executor, UntypedValueExt};
 use crate::{
     core::{TrapCode, UntypedVal},
     engine::bytecode::{Const16, Reg, Sign},
+    ir::ShiftAmount,
     Error,
 };
 use core::num::{NonZeroI32, NonZeroI64, NonZeroU32, NonZeroU64};
@@ -94,48 +95,62 @@ impl Executor<'_> {
         (i64, Instruction::I64AndImm16, execute_i64_and_imm16, UntypedVal::i64_and),
         (i64, Instruction::I64OrImm16, execute_i64_or_imm16, UntypedVal::i64_or),
         (i64, Instruction::I64XorImm16, execute_i64_xor_imm16, UntypedVal::i64_xor),
+    }
+}
 
-        (i32, Instruction::I32ShlImm, execute_i32_shl_imm, UntypedVal::i32_shl),
-        (i32, Instruction::I32ShrUImm, execute_i32_shr_u_imm, UntypedVal::i32_shr_u),
-        (i32, Instruction::I32ShrSImm, execute_i32_shr_s_imm, UntypedVal::i32_shr_s),
-        (i32, Instruction::I32RotlImm, execute_i32_rotl_imm, UntypedVal::i32_rotl),
-        (i32, Instruction::I32RotrImm, execute_i32_rotr_imm, UntypedVal::i32_rotr),
-
-        (i64, Instruction::I64ShlImm, execute_i64_shl_imm, UntypedVal::i64_shl),
-        (i64, Instruction::I64ShrUImm, execute_i64_shr_u_imm, UntypedVal::i64_shr_u),
-        (i64, Instruction::I64ShrSImm, execute_i64_shr_s_imm, UntypedVal::i64_shr_s),
-        (i64, Instruction::I64RotlImm, execute_i64_rotl_imm, UntypedVal::i64_rotl),
-        (i64, Instruction::I64RotrImm, execute_i64_rotr_imm, UntypedVal::i64_rotr),
+macro_rules! impl_shift_by {
+    ( $( ($ty:ty, Instruction::$var_name:ident, $fn_name:ident, $op:expr) ),* $(,)? ) => {
+        $(
+            #[doc = concat!("Executes an [`Instruction::", stringify!($var_name), "`].")]
+            pub fn $fn_name(&mut self, result: Reg, lhs: Reg, rhs: ShiftAmount<$ty>) {
+                self.execute_shift_by(result, lhs, rhs, $op)
+            }
+        )*
+    };
+}
+impl Executor<'_> {
+    impl_shift_by! {
+        (i32, Instruction::I32ShlBy, execute_i32_shl_by, UntypedVal::i32_shl),
+        (i32, Instruction::I32ShrUBy, execute_i32_shr_u_by, UntypedVal::i32_shr_u),
+        (i32, Instruction::I32ShrSBy, execute_i32_shr_s_by, UntypedVal::i32_shr_s),
+        (i32, Instruction::I32RotlBy, execute_i32_rotl_by, UntypedVal::i32_rotl),
+        (i32, Instruction::I32RotrBy, execute_i32_rotr_by, UntypedVal::i32_rotr),
+
+        (i64, Instruction::I64ShlBy, execute_i64_shl_by, UntypedVal::i64_shl),
+        (i64, Instruction::I64ShrUBy, execute_i64_shr_u_by, UntypedVal::i64_shr_u),
+        (i64, Instruction::I64ShrSBy, execute_i64_shr_s_by, UntypedVal::i64_shr_s),
+        (i64, Instruction::I64RotlBy, execute_i64_rotl_by, UntypedVal::i64_rotl),
+        (i64, Instruction::I64RotrBy, execute_i64_rotr_by, UntypedVal::i64_rotr),
 
     }
 }
 
-macro_rules! impl_binary_imm16_rev {
+macro_rules! impl_binary_imm16_lhs {
     ( $( ($ty:ty, Instruction::$var_name:ident, $fn_name:ident, $op:expr) ),* $(,)? ) => {
         $(
             #[doc = concat!("Executes an [`Instruction::", stringify!($var_name), "`].")]
             pub fn $fn_name(&mut self, result: Reg, lhs: Const16<$ty>, rhs: Reg) {
-                self.execute_binary_imm16_rev(result, lhs, rhs, $op)
+                self.execute_binary_imm16_lhs(result, lhs, rhs, $op)
             }
         )*
     };
 }
 impl Executor<'_> {
-    impl_binary_imm16_rev! {
-        (i32, Instruction::I32SubImm16Rev, execute_i32_sub_imm16_rev, UntypedVal::i32_sub),
-        (i64, Instruction::I64SubImm16Rev, execute_i64_sub_imm16_rev, UntypedVal::i64_sub),
-
-        (i32, Instruction::I32ShlImm16Rev, execute_i32_shl_imm16_rev, UntypedVal::i32_shl),
-        (i32, Instruction::I32ShrUImm16Rev, execute_i32_shr_u_imm16_rev, UntypedVal::i32_shr_u),
-        (i32, Instruction::I32ShrSImm16Rev, execute_i32_shr_s_imm16_rev, UntypedVal::i32_shr_s),
-        (i32, Instruction::I32RotlImm16Rev, execute_i32_rotl_imm16_rev, UntypedVal::i32_rotl),
-        (i32, Instruction::I32RotrImm16Rev, execute_i32_rotr_imm16_rev, UntypedVal::i32_rotr),
-
-        (i64, Instruction::I64ShlImm16Rev, execute_i64_shl_imm16_rev, UntypedVal::i64_shl),
-        (i64, Instruction::I64ShrUImm16Rev, execute_i64_shr_u_imm16_rev, UntypedVal::i64_shr_u),
-        (i64, Instruction::I64ShrSImm16Rev, execute_i64_shr_s_imm16_rev, UntypedVal::i64_shr_s),
-        (i64, Instruction::I64RotlImm16Rev, execute_i64_rotl_imm16_rev, UntypedVal::i64_rotl),
-        (i64, Instruction::I64RotrImm16Rev, execute_i64_rotr_imm16_rev, UntypedVal::i64_rotr),
+    impl_binary_imm16_lhs! {
+        (i32, Instruction::I32SubImm16Lhs, execute_i32_sub_imm16_lhs, UntypedVal::i32_sub),
+        (i64, Instruction::I64SubImm16Lhs, execute_i64_sub_imm16_lhs, UntypedVal::i64_sub),
+
+        (i32, Instruction::I32ShlImm16, execute_i32_shl_imm16, UntypedVal::i32_shl),
+        (i32, Instruction::I32ShrUImm16, execute_i32_shr_u_imm16, UntypedVal::i32_shr_u),
+        (i32, Instruction::I32ShrSImm16, execute_i32_shr_s_imm16, UntypedVal::i32_shr_s),
+        (i32, Instruction::I32RotlImm16, execute_i32_rotl_imm16, UntypedVal::i32_rotl),
+        (i32, Instruction::I32RotrImm16, execute_i32_rotr_imm16, UntypedVal::i32_rotr),
+
+        (i64, Instruction::I64ShlImm16, execute_i64_shl_imm16, UntypedVal::i64_shl),
+        (i64, Instruction::I64ShrUImm16, execute_i64_shr_u_imm16, UntypedVal::i64_shr_u),
+        (i64, Instruction::I64ShrSImm16, execute_i64_shr_s_imm16, UntypedVal::i64_shr_s),
+        (i64, Instruction::I64RotlImm16, execute_i64_rotl_imm16, UntypedVal::i64_rotl),
+        (i64, Instruction::I64RotrImm16, execute_i64_rotr_imm16, UntypedVal::i64_rotr),
     }
 }
 
@@ -230,67 +245,67 @@ impl DivRemExt for UntypedVal {
     }
 }
 
-macro_rules! impl_divrem_s_imm16 {
+macro_rules! impl_divrem_s_imm16_rhs {
     ( $( ($ty:ty, Instruction::$var_name:ident, $fn_name:ident, $op:expr) ),* $(,)? ) => {
         $(
             #[doc = concat!("Executes an [`Instruction::", stringify!($var_name), "`].")]
             pub fn $fn_name(&mut self, result: Reg, lhs: Reg, rhs: Const16<$ty>) -> Result<(), Error> {
-                self.try_execute_divrem_imm16(result, lhs, rhs, $op)
+                self.try_execute_divrem_imm16_rhs(result, lhs, rhs, $op)
             }
         )*
     };
 }
 impl Executor<'_> {
-    impl_divrem_s_imm16! {
-        (NonZeroI32, Instruction::I32DivSImm16, execute_i32_div_s_imm16, <UntypedVal as DivRemExt>::i32_div_s),
-        (NonZeroI32, Instruction::I32RemSImm16, execute_i32_rem_s_imm16, <UntypedVal as DivRemExt>::i32_rem_s),
+    impl_divrem_s_imm16_rhs! {
+        (NonZeroI32, Instruction::I32DivSImm16Rhs, execute_i32_div_s_imm16_rhs, <UntypedVal as DivRemExt>::i32_div_s),
+        (NonZeroI32, Instruction::I32RemSImm16Rhs, execute_i32_rem_s_imm16_rhs, <UntypedVal as DivRemExt>::i32_rem_s),
 
-        (NonZeroI64, Instruction::I64DivSImm16, execute_i64_div_s_imm16, <UntypedVal as DivRemExt>::i64_div_s),
-        (NonZeroI64, Instruction::I64RemSImm16, execute_i64_rem_s_imm16, <UntypedVal as DivRemExt>::i64_rem_s),
+        (NonZeroI64, Instruction::I64DivSImm16Rhs, execute_i64_div_s_imm16_rhs, <UntypedVal as DivRemExt>::i64_div_s),
+        (NonZeroI64, Instruction::I64RemSImm16Rhs, execute_i64_rem_s_imm16_rhs, <UntypedVal as DivRemExt>::i64_rem_s),
     }
 }
 
-macro_rules! impl_divrem_u_imm16 {
+macro_rules! impl_divrem_u_imm16_rhs {
     ( $( ($ty:ty, Instruction::$var_name:ident, $fn_name:ident, $op:expr) ),* $(,)? ) => {
         $(
             #[doc = concat!("Executes an [`Instruction::", stringify!($var_name), "`].")]
             pub fn $fn_name(&mut self, result: Reg, lhs: Reg, rhs: Const16<$ty>) {
-                self.execute_divrem_imm16(result, lhs, rhs, $op)
+                self.execute_divrem_imm16_rhs(result, lhs, rhs, $op)
             }
         )*
     };
 }
 impl Executor<'_> {
-    impl_divrem_u_imm16! {
-        (NonZeroU32, Instruction::I32DivUImm16, execute_i32_div_u_imm16, <UntypedVal as DivRemExt>::i32_div_u),
-        (NonZeroU32, Instruction::I32RemUImm16, execute_i32_rem_u_imm16, <UntypedVal as DivRemExt>::i32_rem_u),
+    impl_divrem_u_imm16_rhs! {
+        (NonZeroU32, Instruction::I32DivUImm16Rhs, execute_i32_div_u_imm16_rhs, <UntypedVal as DivRemExt>::i32_div_u),
+        (NonZeroU32, Instruction::I32RemUImm16Rhs, execute_i32_rem_u_imm16_rhs, <UntypedVal as DivRemExt>::i32_rem_u),
 
-        (NonZeroU64, Instruction::I64DivUImm16, execute_i64_div_u_imm16, <UntypedVal as DivRemExt>::i64_div_u),
-        (NonZeroU64, Instruction::I64RemUImm16, execute_i64_rem_u_imm16, <UntypedVal as DivRemExt>::i64_rem_u),
+        (NonZeroU64, Instruction::I64DivUImm16Rhs, execute_i64_div_u_imm16_rhs, <UntypedVal as DivRemExt>::i64_div_u),
+        (NonZeroU64, Instruction::I64RemUImm16Rhs, execute_i64_rem_u_imm16_rhs, <UntypedVal as DivRemExt>::i64_rem_u),
     }
 }
 
-macro_rules! impl_fallible_binary_imm16_rev {
+macro_rules! impl_fallible_binary_imm16_lhs {
     ( $( ($ty:ty, Instruction::$var_name:ident, $fn_name:ident, $op:expr) ),* $(,)? ) => {
         $(
             #[doc = concat!("Executes an [`Instruction::", stringify!($var_name), "`].")]
             pub fn $fn_name(&mut self, result: Reg, lhs: Const16<$ty>, rhs: Reg) -> Result<(), Error> {
-                self.try_execute_binary_imm16_rev(result, lhs, rhs, $op).map_err(Into::into)
+                self.try_execute_binary_imm16_lhs(result, lhs, rhs, $op).map_err(Into::into)
             }
         )*
     };
 }
 impl Executor<'_> {
-    impl_fallible_binary_imm16_rev! {
-        (i32, Instruction::I32DivSImm16Rev, execute_i32_div_s_imm16_rev, UntypedVal::i32_div_s),
-        (u32, Instruction::I32DivUImm16Rev, execute_i32_div_u_imm16_rev, UntypedVal::i32_div_u),
-        (i32, Instruction::I32RemSImm16Rev, execute_i32_rem_s_imm16_rev, UntypedVal::i32_rem_s),
-        (u32, Instruction::I32RemUImm16Rev, execute_i32_rem_u_imm16_rev, UntypedVal::i32_rem_u),
-
-        (i64, Instruction::I64DivSImm16Rev, execute_i64_div_s_imm16_rev, UntypedVal::i64_div_s),
-        (u64, Instruction::I64DivUImm16Rev, execute_i64_div_u_imm16_rev, UntypedVal::i64_div_u),
-        (i64, Instruction::I64RemSImm16Rev, execute_i64_rem_s_imm16_rev, UntypedVal::i64_rem_s),
-        (u64, Instruction::I64RemUImm16Rev, execute_i64_rem_u_imm16_rev, UntypedVal::i64_rem_u),
+    impl_fallible_binary_imm16_lhs! {
+        (i32, Instruction::I32DivSImm16Lhs, execute_i32_div_s_imm16_lhs, UntypedVal::i32_div_s),
+        (u32, Instruction::I32DivUImm16Lhs, execute_i32_div_u_imm16_lhs, UntypedVal::i32_div_u),
+        (i32, Instruction::I32RemSImm16Lhs, execute_i32_rem_s_imm16_lhs, UntypedVal::i32_rem_s),
+        (u32, Instruction::I32RemUImm16Lhs, execute_i32_rem_u_imm16_lhs, UntypedVal::i32_rem_u),
+
+        (i64, Instruction::I64DivSImm16Lhs, execute_i64_div_s_imm16_lhs, UntypedVal::i64_div_s),
+        (u64, Instruction::I64DivUImm16Lhs, execute_i64_div_u_imm16_lhs, UntypedVal::i64_div_u),
+        (i64, Instruction::I64RemSImm16Lhs, execute_i64_rem_s_imm16_lhs, UntypedVal::i64_rem_s),
+        (u64, Instruction::I64RemUImm16Lhs, execute_i64_rem_u_imm16_lhs, UntypedVal::i64_rem_u),
     }
 }
 

crates/wasmi/src/engine/translator/mod.rs

@@ -50,7 +50,7 @@ use crate::{
         BlockType,
         EngineFunc,
     },
-    ir::AnyConst16,
+    ir::{AnyConst16, IntoShiftAmount, ShiftAmount},
     module::{FuncIdx, FuncTypeIdx, ModuleHeader},
     Engine,
     Error,
@@ -1675,13 +1675,13 @@ impl FuncTranslator {
     fn translate_shift<T>(
         &mut self,
         make_instr: fn(result: Reg, lhs: Reg, rhs: Reg) -> Instruction,
-        make_instr_imm: fn(result: Reg, lhs: Reg, rhs: Const16<T>) -> Instruction,
-        make_instr_imm16_rev: fn(result: Reg, lhs: Const16<T>, rhs: Reg) -> Instruction,
+        make_instr_by: fn(result: Reg, lhs: Reg, rhs: ShiftAmount<T>) -> Instruction,
+        make_instr_imm16: fn(result: Reg, lhs: Const16<T>, rhs: Reg) -> Instruction,
         consteval: fn(TypedVal, TypedVal) -> TypedVal,
         make_instr_imm_reg_opt: fn(&mut Self, lhs: T, rhs: Reg) -> Result<bool, Error>,
     ) -> Result<(), Error>
     where
-        T: WasmInteger,
+        T: WasmInteger + IntoShiftAmount,
         Const16<T>: From<i16>,
     {
         bail_unreachable!(self);
@@ -1690,17 +1690,13 @@ impl FuncTranslator {
                 self.push_binary_instr(lhs, rhs, make_instr)
             }
             (TypedProvider::Register(lhs), TypedProvider::Const(rhs)) => {
-                let rhs = T::from(rhs).as_shift_amount();
-                if rhs == 0 {
+                let Some(rhs) = T::from(rhs).into_shift_amount() else {
                     // Optimization: Shifting or rotating by zero bits is a no-op.
                     self.alloc.stack.push_register(lhs)?;
                     return Ok(());
-                }
+                };
                 let result = self.alloc.stack.push_dynamic()?;
-                self.push_fueled_instr(
-                    make_instr_imm(result, lhs, <Const16<T>>::from(rhs)),
-                    FuelCosts::base,
-                )?;
+                self.push_fueled_instr(make_instr_by(result, lhs, rhs), FuelCosts::base)?;
                 Ok(())
             }
             (TypedProvider::Const(lhs), TypedProvider::Register(rhs)) => {
@@ -1713,7 +1709,7 @@ impl FuncTranslator {
                     self.alloc.stack.push_const(lhs);
                     return Ok(());
                 }
-                if self.try_push_binary_instr_imm16_rev(T::from(lhs), rhs, make_instr_imm16_rev)? {
+                if self.try_push_binary_instr_imm16_rev(T::from(lhs), rhs, make_instr_imm16)? {
                     // Optimization was applied: return early.
                     return Ok(());
                 }

crates/wasmi/src/engine/translator/tests/mod.rs

@@ -244,7 +244,8 @@ where
     TranslationTest::from_wat(&wasm)
 }
 
-fn test_binary_reg_imm16<T>(
+/// Variant of [`test_binary_reg_imm16`] where the `rhs` operand is an immediate value.
+fn test_binary_reg_imm16_rhs<T>(
     wasm_op: WasmOp,
     value: T,
     make_instr: fn(result: Reg, lhs: Reg, rhs: Const16<T>) -> Instruction,
@@ -262,8 +263,8 @@ fn test_binary_reg_imm16<T>(
     test_binary_reg_imm_with(wasm_op, value, expected).run()
 }
 
-/// Variant of [`test_binary_reg_imm16`] where both operands are swapped.
-fn test_binary_reg_imm16_rev<T>(
+/// Variant of [`test_binary_reg_imm16`] where the `lhs` operand is an immediate value.
+fn test_binary_reg_imm16_lhs<T>(
     wasm_op: WasmOp,
     value: T,
     make_instr: fn(result: Reg, lhs: Const16<T>, rhs: Reg) -> Instruction,
@@ -278,7 +279,7 @@ fn test_binary_reg_imm16_rev<T>(
         make_instr(Reg::from(1), immediate, Reg::from(0)),
         Instruction::return_reg(1),
     ];
-    test_binary_reg_imm_rev_with(wasm_op, value, expected).run()
+    test_binary_reg_imm_lhs_with(wasm_op, value, expected).run()
 }
 
 fn test_binary_reg_imm32<T>(
@@ -299,7 +300,7 @@ fn test_binary_reg_imm32<T>(
 }
 
 /// Variant of [`test_binary_reg_imm32`] where both operands are swapped.
-fn test_binary_reg_imm32_rev<T>(
+fn test_binary_reg_imm32_lhs<T>(
     wasm_op: WasmOp,
     value: T,
     make_instr: fn(result: Reg, lhs: Reg, rhs: Reg) -> Instruction,
@@ -317,7 +318,7 @@ fn test_binary_reg_imm32_rev<T>(
 }
 
 /// Variant of [`test_binary_reg_imm32`] where both operands are swapped.
-fn test_binary_reg_imm32_rev_commutative<T>(
+fn test_binary_reg_imm32_lhs_commutative<T>(
     wasm_op: WasmOp,
     value: T,
     make_instr: fn(result: Reg, lhs: Reg, rhs: Reg) -> Instruction,
@@ -346,7 +347,7 @@ where
     testcase
 }
 
-fn test_binary_reg_imm_rev_with<T, E>(wasm_op: WasmOp, value: T, expected: E) -> TranslationTest
+fn test_binary_reg_imm_lhs_with<T, E>(wasm_op: WasmOp, value: T, expected: E) -> TranslationTest
 where
     T: Copy,
     DisplayWasm<T>: Display,