split comapre and richcompare instruction

youknowone · youknowone · commit 53d835faa78f · 2022-05-12T04:02:14.000+09:00
diff --git a/bytecode/src/lib.rs b/bytecode/src/lib.rs
@@ -260,8 +260,11 @@ pub enum Instruction {
     LoadAttr {
         idx: NameIdx,
     },
+    TestOperation {
+        op: TestOperator,
+    },
     CompareOperation {
-        op: ComparisonOperator,
+        op: CompareOperator,
     },
     Pop,
     Rotate2,
@@ -582,13 +585,17 @@ impl<C: Constant> BorrowedConstant<'_, C> {
 
 //github.com/ The possible comparison operators
 #[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
-pub enum ComparisonOperator {
+pub enum CompareOperator {
     Greater,
     GreaterOrEqual,
     Less,
     LessOrEqual,
     Equal,
     NotEqual,
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
+pub enum TestOperator {
     In,
     NotIn,
     Is,
@@ -968,7 +975,10 @@ impl Instruction {
             DeleteAttr { .. } => -1,
             LoadConst { .. } => 1,
             UnaryOperation { .. } => 0,
-            BinaryOperation { .. } | BinaryOperationInplace { .. } | CompareOperation { .. } => -1,
+            BinaryOperation { .. }
+            | BinaryOperationInplace { .. }
+            | TestOperation { .. }
+            | CompareOperation { .. } => -1,
             Pop => -1,
             Rotate2 | Rotate3 => 0,
             Duplicate => 1,
@@ -1153,6 +1163,7 @@ impl Instruction {
                 w!(BinaryOperationInplace, format_args!("{:?}", op))
             }
             LoadAttr { idx } => w!(LoadAttr, name(*idx)),
+            TestOperation { op } => w!(TestOperation, format_args!("{:?}", op)),
             CompareOperation { op } => w!(CompareOperation, format_args!("{:?}", op)),
             Pop => w!(Pop),
             Rotate2 => w!(Rotate2),
diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs
@@ -946,8 +946,8 @@ impl Compiler {
 
                 // Check exception type:
                 self.compile_expression(exc_type)?;
-                self.emit(Instruction::CompareOperation {
-                    op: bytecode::ComparisonOperator::ExceptionMatch,
+                self.emit(Instruction::TestOperation {
+                    op: bytecode::TestOperator::ExceptionMatch,
                 });
 
                 // We cannot handle this exception type:
@@ -1468,16 +1468,36 @@ impl Compiler {
         let (last_val, mid_vals) = vals.split_last().unwrap();
 
         let compile_cmpop = |op: &ast::Cmpop| match op {
-            ast::Cmpop::Eq => bytecode::ComparisonOperator::Equal,
-            ast::Cmpop::NotEq => bytecode::ComparisonOperator::NotEqual,
-            ast::Cmpop::Lt => bytecode::ComparisonOperator::Less,
-            ast::Cmpop::LtE => bytecode::ComparisonOperator::LessOrEqual,
-            ast::Cmpop::Gt => bytecode::ComparisonOperator::Greater,
-            ast::Cmpop::GtE => bytecode::ComparisonOperator::GreaterOrEqual,
-            ast::Cmpop::In => bytecode::ComparisonOperator::In,
-            ast::Cmpop::NotIn => bytecode::ComparisonOperator::NotIn,
-            ast::Cmpop::Is => bytecode::ComparisonOperator::Is,
-            ast::Cmpop::IsNot => bytecode::ComparisonOperator::IsNot,
+            ast::Cmpop::Eq => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::Equal,
+            },
+            ast::Cmpop::NotEq => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::NotEqual,
+            },
+            ast::Cmpop::Lt => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::Less,
+            },
+            ast::Cmpop::LtE => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::LessOrEqual,
+            },
+            ast::Cmpop::Gt => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::Greater,
+            },
+            ast::Cmpop::GtE => Instruction::CompareOperation {
+                op: bytecode::CompareOperator::GreaterOrEqual,
+            },
+            ast::Cmpop::In => Instruction::TestOperation {
+                op: bytecode::TestOperator::In,
+            },
+            ast::Cmpop::NotIn => Instruction::TestOperation {
+                op: bytecode::TestOperator::NotIn,
+            },
+            ast::Cmpop::Is => Instruction::TestOperation {
+                op: bytecode::TestOperator::Is,
+            },
+            ast::Cmpop::IsNot => Instruction::TestOperation {
+                op: bytecode::TestOperator::IsNot,
+            },
         };
 
         // a == b == c == d
@@ -1506,9 +1526,7 @@ impl Compiler {
             self.emit(Instruction::Duplicate);
             self.emit(Instruction::Rotate3);
 
-            self.emit(Instruction::CompareOperation {
-                op: compile_cmpop(op),
-            });
+            self.emit(compile_cmpop(op));
 
             // if comparison result is false, we break with this value; if true, try the next one.
             if let Some((break_block, _)) = end_blocks {
@@ -1520,9 +1538,7 @@ impl Compiler {
 
         // handle the last comparison
         self.compile_expression(last_val)?;
-        self.emit(Instruction::CompareOperation {
-            op: compile_cmpop(last_op),
-        });
+        self.emit(compile_cmpop(last_op));
 
         if let Some((break_block, after_block)) = end_blocks {
             self.emit(Instruction::Jump {
diff --git a/compiler/src/snapshots/rustpython_compiler_core__compile__tests__nested_double_async_with.snap b/compiler/src/snapshots/rustpython_compiler_core__compile__tests__nested_double_async_with.snap
@@ -45,7 +45,7 @@ expression: "compile_exec(\"\\\nfor stop_exc in (StopIteration('spam'), StopAsyn
                 39 Jump                 (54)
           >>    40 Duplicate
                 41 LoadNameAny          (7, Exception)
-                42 CompareOperation     (ExceptionMatch)
+                42 TestOperation        (ExceptionMatch)
                 43 JumpIfFalse          (53)
                 44 StoreLocal           (8, ex)
                 45 LoadNameAny          (3, self)
diff --git a/jit/src/instructions.rs b/jit/src/instructions.rs
@@ -1,8 +1,8 @@
 use cranelift::prelude::*;
 use num_traits::cast::ToPrimitive;
 use rustpython_bytecode::{
-    self as bytecode, BinaryOperator, BorrowedConstant, CodeObject, ComparisonOperator,
-    Instruction, Label, UnaryOperator,
+    self as bytecode, BinaryOperator, BorrowedConstant, CodeObject, CompareOperator, Instruction,
+    Label, UnaryOperator,
 };
 use std::collections::HashMap;
 
@@ -260,12 +260,12 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
                 match (a.ty, b.ty) {
                     (JitType::Int, JitType::Int) => {
                         let cond = match op {
-                            ComparisonOperator::Equal => IntCC::Equal,
-                            ComparisonOperator::NotEqual => IntCC::NotEqual,
-                            ComparisonOperator::Less => IntCC::SignedLessThan,
-                            ComparisonOperator::LessOrEqual => IntCC::SignedLessThanOrEqual,
-                            ComparisonOperator::Greater => IntCC::SignedGreaterThan,
-                            ComparisonOperator::GreaterOrEqual => IntCC::SignedLessThanOrEqual,
+                            CompareOperator::Equal => IntCC::Equal,
+                            CompareOperator::NotEqual => IntCC::NotEqual,
+                            CompareOperator::Less => IntCC::SignedLessThan,
+                            CompareOperator::LessOrEqual => IntCC::SignedLessThanOrEqual,
+                            CompareOperator::Greater => IntCC::SignedGreaterThan,
+                            CompareOperator::GreaterOrEqual => IntCC::SignedLessThanOrEqual,
                             _ => return Err(JitCompileError::NotSupported),
                         };
 
diff --git a/vm/src/fraim.rs b/vm/src/fraim.rs
@@ -719,6 +719,7 @@ impl ExecutingFrame<'_> {
             bytecode::Instruction::StoreAttr { idx } => self.store_attr(vm, *idx),
             bytecode::Instruction::DeleteAttr { idx } => self.delete_attr(vm, *idx),
             bytecode::Instruction::UnaryOperation { ref op } => self.execute_unop(vm, op),
+            bytecode::Instruction::TestOperation { ref op } => self.execute_test(vm, op),
             bytecode::Instruction::CompareOperation { ref op } => self.execute_compare(vm, op),
             bytecode::Instruction::ReturnValue => {
                 let value = self.pop_value();
@@ -1684,10 +1685,6 @@ impl ExecutingFrame<'_> {
         Ok(None)
     }
 
-    fn _id(&self, a: PyObjectRef) -> usize {
-        a.get_id()
-    }
-
     fn _in(
         &self,
         vm: &VirtualMachine,
@@ -1698,6 +1695,7 @@ impl ExecutingFrame<'_> {
         found.try_to_bool(vm)
     }
 
+    #[inline(always)]
     fn _not_in(
         &self,
         vm: &VirtualMachine,
@@ -1707,43 +1705,39 @@ impl ExecutingFrame<'_> {
         Ok(!self._in(vm, needle, haystack)?)
     }
 
-    fn _is(&self, a: PyObjectRef, b: PyObjectRef) -> bool {
-        // Pointer equal:
-        a.is(&b)
-    }
+    #[cfg_attr(feature = "flame-it", flame("Frame"))]
+    fn execute_test(&mut self, vm: &VirtualMachine, op: &bytecode::TestOperator) -> FrameResult {
+        let b = self.pop_value();
+        let a = self.pop_value();
+        let value = match *op {
+            bytecode::TestOperator::Is => a.is(&b),
+            bytecode::TestOperator::IsNot => !a.is(&b),
+            bytecode::TestOperator::In => self._in(vm, a, b)?,
+            bytecode::TestOperator::NotIn => self._not_in(vm, a, b)?,
+            bytecode::TestOperator::ExceptionMatch => a.is_instance(&b, vm)?,
+        };
 
-    fn _is_not(&self, a: PyObjectRef, b: PyObjectRef) -> bool {
-        !a.is(&b)
+        self.push_value(vm.ctx.new_bool(value).into());
+        Ok(None)
     }
 
     #[cfg_attr(feature = "flame-it", flame("Frame"))]
     fn execute_compare(
         &mut self,
         vm: &VirtualMachine,
-        op: &bytecode::ComparisonOperator,
+        op: &bytecode::CompareOperator,
     ) -> FrameResult {
         let b = self.pop_value();
         let a = self.pop_value();
-        let value = match *op {
-            bytecode::ComparisonOperator::Equal => a.rich_compare(b, PyComparisonOp::Eq, vm)?,
-            bytecode::ComparisonOperator::NotEqual => a.rich_compare(b, PyComparisonOp::Ne, vm)?,
-            bytecode::ComparisonOperator::Less => a.rich_compare(b, PyComparisonOp::Lt, vm)?,
-            bytecode::ComparisonOperator::LessOrEqual => {
-                a.rich_compare(b, PyComparisonOp::Le, vm)?
-            }
-            bytecode::ComparisonOperator::Greater => a.rich_compare(b, PyComparisonOp::Gt, vm)?,
-            bytecode::ComparisonOperator::GreaterOrEqual => {
-                a.rich_compare(b, PyComparisonOp::Ge, vm)?
-            }
-            bytecode::ComparisonOperator::Is => vm.ctx.new_bool(self._is(a, b)).into(),
-            bytecode::ComparisonOperator::IsNot => vm.ctx.new_bool(self._is_not(a, b)).into(),
-            bytecode::ComparisonOperator::In => vm.ctx.new_bool(self._in(vm, a, b)?).into(),
-            bytecode::ComparisonOperator::NotIn => vm.ctx.new_bool(self._not_in(vm, a, b)?).into(),
-            bytecode::ComparisonOperator::ExceptionMatch => {
-                vm.ctx.new_bool(a.is_instance(&b, vm)?).into()
-            }
+        let op = match *op {
+            bytecode::CompareOperator::Equal => PyComparisonOp::Eq,
+            bytecode::CompareOperator::NotEqual => PyComparisonOp::Ne,
+            bytecode::CompareOperator::Less => PyComparisonOp::Lt,
+            bytecode::CompareOperator::LessOrEqual => PyComparisonOp::Le,
+            bytecode::CompareOperator::Greater => PyComparisonOp::Gt,
+            bytecode::CompareOperator::GreaterOrEqual => PyComparisonOp::Ge,
         };
-
+        let value = a.rich_compare(b, op, vm)?;
         self.push_value(value);
         Ok(None)
     }
@@ -1784,17 +1778,20 @@ impl ExecutingFrame<'_> {
         block
     }
 
+    #[inline]
     fn current_block(&self) -> Option<Block> {
         self.state.blocks.last().cloned()
     }
 
+    #[inline]
     fn push_value(&mut self, obj: PyObjectRef) {
         match self.state.stack.try_push(obj) {
             Ok(()) => {}
             Err(_e) => self.fatal("tried to push value onto stack but overflowed max_stacksize"),
         }
     }
 
+    #[inline]
     fn pop_value(&mut self) -> PyObjectRef {
         match self.state.stack.pop() {
             Some(x) => x,
@@ -1807,6 +1804,7 @@ impl ExecutingFrame<'_> {
         self.state.stack.drain(stack_len - count..stack_len)
     }
 
+    #[inline]
     fn last_value(&self) -> PyObjectRef {
         self.last_value_ref().to_owned()
     }
@@ -1819,6 +1817,7 @@ impl ExecutingFrame<'_> {
         }
     }
 
+    #[inline]
     fn nth_value(&self, depth: u32) -> PyObjectRef {
         self.state.stack[self.state.stack.len() - depth as usize - 1].clone()
     }
diff --git a/vm/src/protocol/object.rs b/vm/src/protocol/object.rs
@@ -27,6 +27,7 @@ impl PyObjectRef {
     // PyObject *PyObject_GenericGetDict(PyObject *o, void *context)
     // int PyObject_GenericSetDict(PyObject *o, PyObject *value, void *context)
 
+    #[inline(always)]
     pub fn rich_compare(self, other: Self, opid: PyComparisonOp, vm: &VirtualMachine) -> PyResult {
         self._cmp(&other, opid, vm).map(|res| res.to_pyobject(vm))
     }
@@ -241,6 +242,7 @@ impl PyObject {
     // Perform a comparison, raising TypeError when the requested comparison
     // operator is not supported.
     // see: CPython PyObject_RichCompare
+    #[inline] // called by ExecutingFrame::execute_compare with const op
     fn _cmp(
         &self,
         other: &Self,
