diff --git a/Lib/test/test_exceptions.py b/Lib/test/test_exceptions.py index ceb94df324e..0c273935cb4 100644 --- a/Lib/test/test_exceptions.py +++ b/Lib/test/test_exceptions.py @@ -1798,7 +1798,6 @@ def g(): next(i) next(i) - @unittest.expectedFailure # TODO: RUSTPYTHON @unittest.skipUnless(__debug__, "Won't work if __debug__ is False") def test_assert_shadowing(self): # Shadowing AssertionError would cause the assert statement to diff --git a/crates/codegen/src/compile.rs b/crates/codegen/src/compile.rs index 5ace63c53eb..40d92bf6842 100644 --- a/crates/codegen/src/compile.rs +++ b/crates/codegen/src/compile.rs @@ -36,6 +36,51 @@ use rustpython_compiler_core::{ }; use rustpython_wtf8::Wtf8Buf; +/// Extension trait for `ast::Expr` to add constant checking methods +trait ExprExt { + /// Check if an expression is a constant literal + fn is_constant(&self) -> bool; + + /// Check if a slice expression has all constant elements + fn is_constant_slice(&self) -> bool; + + /// Check if we should use BINARY_SLICE/STORE_SLICE optimization + fn should_use_slice_optimization(&self) -> bool; +} + +impl ExprExt for ast::Expr { + fn is_constant(&self) -> bool { + matches!( + self, + ast::Expr::NumberLiteral(_) + | ast::Expr::StringLiteral(_) + | ast::Expr::BytesLiteral(_) + | ast::Expr::NoneLiteral(_) + | ast::Expr::BooleanLiteral(_) + | ast::Expr::EllipsisLiteral(_) + ) + } + + fn is_constant_slice(&self) -> bool { + match self { + ast::Expr::Slice(s) => { + let lower_const = + s.lower.is_none() || s.lower.as_deref().is_some_and(|e| e.is_constant()); + let upper_const = + s.upper.is_none() || s.upper.as_deref().is_some_and(|e| e.is_constant()); + let step_const = + s.step.is_none() || s.step.as_deref().is_some_and(|e| e.is_constant()); + lower_const && upper_const && step_const + } + _ => false, + } + } + + fn should_use_slice_optimization(&self) -> bool { + !self.is_constant_slice() && matches!(self, ast::Expr::Slice(s) if s.step.is_none()) + } +} + const MAXBLOCKS: usize = 20; #[derive(Debug, Clone, Copy)] @@ -425,39 +470,25 @@ impl Compiler { } } - /// Check if the slice is a two-element slice (no step) - // = is_two_element_slice - const fn is_two_element_slice(slice: &ast::Expr) -> bool { - matches!(slice, ast::Expr::Slice(s) if s.step.is_none()) - } - - /// Compile a slice expression - // = compiler_slice - fn compile_slice(&mut self, s: &ast::ExprSlice) -> CompileResult { - // Compile lower + /// Compile just start and stop of a slice (for BINARY_SLICE/STORE_SLICE) + // = codegen_slice_two_parts + fn compile_slice_two_parts(&mut self, s: &ast::ExprSlice) -> CompileResult<()> { + // Compile lower (or None) if let Some(lower) = &s.lower { self.compile_expression(lower)?; } else { self.emit_load_const(ConstantData::None); } - // Compile upper + // Compile upper (or None) if let Some(upper) = &s.upper { self.compile_expression(upper)?; } else { self.emit_load_const(ConstantData::None); } - Ok(match &s.step { - Some(step) => { - // Compile step if present - self.compile_expression(step)?; - BuildSliceArgCount::Three - } - None => BuildSliceArgCount::Two, - }) + Ok(()) } - /// Compile a subscript expression // = compiler_subscript fn compile_subscript( @@ -480,27 +511,20 @@ impl Compiler { // VISIT(c, expr, e->v.Subscript.value) self.compile_expression(value)?; - // Handle two-element slice (for Load/Store, not Del) - if Self::is_two_element_slice(slice) && !matches!(ctx, ast::ExprContext::Del) { - let argc = match slice { - ast::Expr::Slice(s) => self.compile_slice(s)?, + // Handle two-element non-constant slice with BINARY_SLICE/STORE_SLICE + if slice.should_use_slice_optimization() && !matches!(ctx, ast::ExprContext::Del) { + match slice { + ast::Expr::Slice(s) => self.compile_slice_two_parts(s)?, _ => unreachable!( - "is_two_element_slice should only return true for ast::Expr::Slice" + "should_use_slice_optimization should only return true for ast::Expr::Slice" ), }; match ctx { ast::ExprContext::Load => { - emit!(self, Instruction::BuildSlice { argc }); - emit!( - self, - Instruction::BinaryOp { - op: BinaryOperator::Subscr - } - ); + emit!(self, Instruction::BinarySlice); } ast::ExprContext::Store => { - emit!(self, Instruction::BuildSlice { argc }); - emit!(self, Instruction::StoreSubscr); + emit!(self, Instruction::StoreSlice); } _ => unreachable!(), } @@ -1269,8 +1293,12 @@ impl Compiler { emit!(self, Instruction::PopJumpIfFalse { target: body_block }); // Raise NotImplementedError - let not_implemented_error = self.name("NotImplementedError"); - emit!(self, Instruction::LoadGlobal(not_implemented_error)); + emit!( + self, + Instruction::LoadCommonConstant { + idx: bytecode::CommonConstant::NotImplementedError + } + ); emit!( self, Instruction::RaiseVarargs { @@ -2345,8 +2373,12 @@ impl Compiler { let after_block = self.new_block(); self.compile_jump_if(test, true, after_block)?; - let assertion_error = self.name("AssertionError"); - emit!(self, Instruction::LoadGlobal(assertion_error)); + emit!( + self, + Instruction::LoadCommonConstant { + idx: bytecode::CommonConstant::AssertionError + } + ); emit!(self, Instruction::PushNull); match msg { Some(e) => { @@ -3326,11 +3358,9 @@ impl Compiler { // ADDOP_I(c, loc, COPY, 1); // ADDOP_JUMP(c, loc, POP_JUMP_IF_NONE, no_match); emit!(self, Instruction::Copy { index: 1 }); - self.emit_load_const(ConstantData::None); - emit!(self, Instruction::IsOp(bytecode::Invert::No)); // is None? emit!( self, - Instruction::PopJumpIfTrue { + Instruction::PopJumpIfNone { target: no_match_block } ); @@ -3455,11 +3485,9 @@ impl Compiler { // Stack: [prev_exc, result, result] // POP_JUMP_IF_NOT_NONE reraise - self.emit_load_const(ConstantData::None); - emit!(self, Instruction::IsOp(bytecode::Invert::Yes)); // is not None? emit!( self, - Instruction::PopJumpIfTrue { + Instruction::PopJumpIfNotNone { target: reraise_block } ); diff --git a/crates/codegen/src/ir.rs b/crates/codegen/src/ir.rs index 59848fb23ba..94db8a91901 100644 --- a/crates/codegen/src/ir.rs +++ b/crates/codegen/src/ir.rs @@ -161,6 +161,7 @@ impl CodeInfo { ) -> crate::InternalResult { if opts.optimize > 0 { self.dce(); + self.peephole_optimize(); } let max_stackdepth = self.max_stackdepth()?; @@ -403,6 +404,70 @@ impl CodeInfo { } } + /// Peephole optimization: combine consecutive instructions into super-instructions + fn peephole_optimize(&mut self) { + for block in &mut self.blocks { + let mut i = 0; + while i + 1 < block.instructions.len() { + let combined = { + let curr = &block.instructions[i]; + let next = &block.instructions[i + 1]; + + // Only combine if both are real instructions (not pseudo) + let (Some(curr_instr), Some(next_instr)) = + (curr.instr.real(), next.instr.real()) + else { + i += 1; + continue; + }; + + match (curr_instr, next_instr) { + // LoadFast + LoadFast -> LoadFastLoadFast (if both indices < 16) + (Instruction::LoadFast(_), Instruction::LoadFast(_)) => { + let idx1 = curr.arg.0; + let idx2 = next.arg.0; + if idx1 < 16 && idx2 < 16 { + let packed = (idx1 << 4) | idx2; + Some(( + Instruction::LoadFastLoadFast { arg: Arg::marker() }, + OpArg(packed), + )) + } else { + None + } + } + // StoreFast + StoreFast -> StoreFastStoreFast (if both indices < 16) + (Instruction::StoreFast(_), Instruction::StoreFast(_)) => { + let idx1 = curr.arg.0; + let idx2 = next.arg.0; + if idx1 < 16 && idx2 < 16 { + let packed = (idx1 << 4) | idx2; + Some(( + Instruction::StoreFastStoreFast { arg: Arg::marker() }, + OpArg(packed), + )) + } else { + None + } + } + _ => None, + } + }; + + if let Some((new_instr, new_arg)) = combined { + // Combine: keep first instruction's location, replace with combined instruction + block.instructions[i].instr = new_instr.into(); + block.instructions[i].arg = new_arg; + // Remove the second instruction + block.instructions.remove(i + 1); + // Don't increment i - check if we can combine again with the next instruction + } else { + i += 1; + } + } + } + } + fn max_stackdepth(&self) -> crate::InternalResult { let mut maxdepth = 0u32; let mut stack = Vec::with_capacity(self.blocks.len()); diff --git a/crates/compiler-core/src/bytecode.rs b/crates/compiler-core/src/bytecode.rs index 79f3a985dc7..d177b728efa 100644 --- a/crates/compiler-core/src/bytecode.rs +++ b/crates/compiler-core/src/bytecode.rs @@ -21,7 +21,7 @@ pub use crate::bytecode::{ encode_load_super_attr_arg, }, oparg::{ - BinaryOperator, BuildSliceArgCount, ComparisonOperator, ConvertValueOparg, + BinaryOperator, BuildSliceArgCount, CommonConstant, ComparisonOperator, ConvertValueOparg, IntrinsicFunction1, IntrinsicFunction2, Invert, Label, MakeFunctionFlags, NameIdx, OpArg, OpArgByte, OpArgState, OpArgType, RaiseKind, ResumeType, SpecialMethod, UnpackExArgs, }, diff --git a/crates/compiler-core/src/bytecode/instruction.rs b/crates/compiler-core/src/bytecode/instruction.rs index 1962e195596..f9c9d17c0ad 100644 --- a/crates/compiler-core/src/bytecode/instruction.rs +++ b/crates/compiler-core/src/bytecode/instruction.rs @@ -4,9 +4,10 @@ use crate::{ bytecode::{ BorrowedConstant, Constant, InstrDisplayContext, oparg::{ - BinaryOperator, BuildSliceArgCount, ComparisonOperator, ConvertValueOparg, - IntrinsicFunction1, IntrinsicFunction2, Invert, Label, MakeFunctionFlags, NameIdx, - OpArg, OpArgByte, OpArgType, RaiseKind, SpecialMethod, UnpackExArgs, + BinaryOperator, BuildSliceArgCount, CommonConstant, ComparisonOperator, + ConvertValueOparg, IntrinsicFunction1, IntrinsicFunction2, Invert, Label, + MakeFunctionFlags, NameIdx, OpArg, OpArgByte, OpArgType, RaiseKind, SpecialMethod, + UnpackExArgs, }, }, marshal::MarshalError, @@ -22,8 +23,8 @@ use crate::{ #[repr(u8)] pub enum Instruction { // No-argument instructions (opcode < HAVE_ARGUMENT=44) - Cache = 0, // Placeholder - BinarySlice = 1, // Placeholder + Cache = 0, // Placeholder + BinarySlice = 1, BuildTemplate = 2, BinaryOpInplaceAddUnicode = 3, // Placeholder CallFunctionEx = 4, @@ -59,7 +60,7 @@ pub enum Instruction { ReturnGenerator = 34, ReturnValue = 35, SetupAnnotations = 36, - StoreSlice = 37, // Placeholder + StoreSlice = 37, StoreSubscr = 38, ToBool = 39, UnaryInvert = 40, @@ -122,7 +123,7 @@ pub enum Instruction { } = 59, CopyFreeVars { count: Arg, - } = 60, // Placeholder + } = 60, DeleteAttr { idx: Arg, } = 61, @@ -168,8 +169,8 @@ pub enum Instruction { idx: Arg, } = 80, LoadCommonConstant { - idx: Arg, - } = 81, // Placeholder + idx: Arg, + } = 81, LoadConst { idx: Arg, } = 82, @@ -180,10 +181,10 @@ pub enum Instruction { LoadFastBorrowLoadFastBorrow { arg: Arg, } = 87, // Placeholder - LoadFastCheck(Arg) = 88, // Placeholder + LoadFastCheck(Arg) = 88, LoadFastLoadFast { arg: Arg, - } = 89, // Placeholder + } = 89, LoadFromDictOrDeref(Arg) = 90, LoadFromDictOrGlobals(Arg) = 91, // Placeholder LoadGlobal(Arg) = 92, @@ -197,7 +198,7 @@ pub enum Instruction { LoadSuperAttr { arg: Arg, } = 96, - MakeCell(Arg) = 97, // Placeholder + MakeCell(Arg) = 97, MapAdd { i: Arg, } = 98, @@ -207,10 +208,10 @@ pub enum Instruction { } = 100, PopJumpIfNone { target: Arg