slice ops and a little bit of peephole #6860
Conversation
📝 WalkthroughWalkthroughThe pull request introduces bytecode-level optimizations and new instruction types. Changes include a slice optimization path using a new ExprExt trait, peephole optimization for fusing consecutive load/store instructions, a CommonConstant enum for frequently-used constants, and VM execution paths for eight new or modified instructions. Changes
Sequence DiagramsequenceDiagram
participant Compiler as Compiler<br/>(compile.rs)
participant CodeGen as Code Generator<br/>(ir.rs)
participant Bytecode as Bytecode<br/>(instruction.rs)
participant Peephole as Peephole Opt.<br/>(ir.rs)
participant VM as VM Executor<br/>(frame.rs)
Compiler->>Compiler: Check is_constant_slice<br/>& should_use_slice_optimization
alt Optimize Slice Path
Compiler->>Bytecode: Emit BinarySlice/StoreSlice
else Standard Path
Compiler->>Bytecode: Emit standard slice instructions
end
Compiler->>CodeGen: Generate code with<br/>LoadFast/StoreFast pairs
CodeGen->>Peephole: Finalize code with<br/>optimization enabled
Peephole->>Peephole: Scan consecutive instructions
alt Fusible Pattern Found
Peephole->>Peephole: Fuse LoadFast+LoadFast<br/>→ LoadFastLoadFast
Peephole->>Peephole: Fuse StoreFast+StoreFast<br/>→ StoreFastStoreFast
end
Peephole->>Bytecode: Output optimized<br/>instructions
VM->>VM: Execute instruction
alt BinarySlice
VM->>VM: Pop container, start, stop
VM->>VM: Create PySlice
VM->>VM: Get item, push result
else LoadFastLoadFast
VM->>VM: Decode indices<br/>(idx1 << 4 | idx2)
VM->>VM: Load both with<br/>unbound checks
VM->>VM: Push both values
else StoreFastStoreFast
VM->>VM: Pop 2 values
VM->>VM: Store to 2 locals
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [+] test: cpython/Lib/test/test_exceptions.py dependencies: dependent tests: (no tests depend on exception) Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
514-531: Guard the optimized slice path from Invalid contexts.
Right nowInvalidcan slip into the optimized branch and hitunreachable!(). It’s safer to limit the optimization to Load/Store and let the fallback path return the syntax error.🐛 Suggested fix
- if slice.should_use_slice_optimization() && !matches!(ctx, ast::ExprContext::Del) { + if slice.should_use_slice_optimization() + && matches!(ctx, ast::ExprContext::Load | ast::ExprContext::Store) + { match slice { ast::Expr::Slice(s) => self.compile_slice_two_parts(s)?, _ => unreachable!( "should_use_slice_optimization should only return true for ast::Expr::Slice" ), }; match ctx { ast::ExprContext::Load => { emit!(self, Instruction::BinarySlice); } ast::ExprContext::Store => { emit!(self, Instruction::StoreSlice); } - _ => unreachable!(), + _ => unreachable!(), } } else {
♻️ Duplicate comments (1)
crates/vm/src/frame.rs (1)
1806-1820: Same packed‑index constraint as LoadFastLoadFast applies here.
🧹 Nitpick comments (2)
crates/codegen/src/ir.rs (1)
407-466: Guard fusion across differing spans/handlers.
Fusing across differentexcept_handleror source spans can alter exception-table coverage or line events. Consider guarding (or asserting) that both instructions share the same span/handler before combining.💡 Suggested guard
let curr = &block.instructions[i]; let next = &block.instructions[i + 1]; + if curr.except_handler != next.except_handler + || curr.location != next.location + || curr.end_location != next.end_location + { + i += 1; + continue; + } + // Only combine if both are real instructions (not pseudo) let (Some(curr_instr), Some(next_instr)) = (curr.instr.real(), next.instr.real()) else {crates/compiler-core/src/bytecode/instruction.rs (1)
171-247: Add disassembly formatting for new/common-constant & fused ops.
Now thatLoadCommonConstant,LoadFastLoadFast, andStoreFastStoreFastare used,fmt_disfalls back toRUSTPYTHON_PLACEHOLDERfor them. Adding explicit cases improves debug output.♻️ Suggested disassembly cases
@@ - Self::LoadConst { idx } => fmt_const("LOAD_CONST", arg, f, idx), + Self::LoadConst { idx } => fmt_const("LOAD_CONST", arg, f, idx), + Self::LoadCommonConstant { idx } => { + let c = idx.get(arg); + write!(f, "{:pad$}({})", "LOAD_COMMON_CONSTANT", c) + } @@ Self::LoadFastAndClear(idx) => w!(LOAD_FAST_AND_CLEAR, varname = idx), + Self::LoadFastLoadFast { .. } => { + let packed = arg.0; + let idx1 = packed >> 4; + let idx2 = packed & 0xF; + write!(f, "LOAD_FAST_LOAD_FAST ({}, {})", varname(idx1), varname(idx2)) + } @@ Self::StoreFast(idx) => w!(STORE_FAST, varname = idx), + Self::StoreFastStoreFast { .. } => { + let packed = arg.0; + let idx1 = packed >> 4; + let idx2 = packed & 0xF; + write!(f, "STORE_FAST_STORE_FAST ({}, {})", varname(idx1), varname(idx2)) + }
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.