GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — slice ops and a little bit of peephole by youknowone · Pull Request #6860 · RustPython/RustPython 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
📦 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:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
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 {
crates/vm/src/frame.rs (1)
1806-1820: Same packed‑index constraint as LoadFastLoadFast applies here.
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)) + }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That's great!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.