More Load instructions #6886
More Load instructions #6886
Conversation
📝 WalkthroughWalkthroughThis PR adds new bytecode instruction variants, a LOAD_FAST→LOAD_FAST_BORROW optimization, modifies class-scope name emission to use a dict-aware load path, updates VM/JIT handlers for the new opcodes, and adds many cspell dictionary entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source (AST/Parser)
participant Compiler as Compiler/Codegen
participant IR as IR optimizer
participant Bytecode as BytecodeEmitter
participant VM as VM/JIT
Source->>Compiler: emit name ops (class scope -> LoadLocals / LoadFromDictOrDeref)
Compiler->>IR: produce instruction stream (LOAD_FAST, etc.)
IR->>IR: optimize_load_fast_borrow() (convert LOAD_FAST -> LOAD_FAST_BORROW when safe)
IR->>Bytecode: finalize and emit new opcodes
Bytecode->>VM: execute bytecode
VM->>VM: handle LoadFastBorrow / LoadFromDictOrDeref at runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
youknowone
force-pushed
the
load-ops
branch
3 times, most recently
from
b78584b to
29ec496
Compare
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin load-ops |
![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/vm/src/frame.rs (1)
1178-1194: Preserve non‑KeyError exceptions from class-dict lookups.
class_dict.get_item(...).ok()converts any error into “not found,” which can mask real exceptions from custom mappings returned by__prepare__. The globals path already treats onlyKeyErroras “miss”; this should do the same.🔧 Suggested fix
- // First try to find in the dict - let value = class_dict.get_item(name, vm).ok(); + // First try to find in the dict + let value = if let Some(dict_obj) = class_dict.downcast_ref::<PyDict>() { + dict_obj.get_item_opt(name, vm)? + } else { + match class_dict.get_item(name, vm) { + Ok(v) => Some(v), + Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error) => None, + Err(e) => return Err(e), + } + };
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
693-795: Make the borrow optimization conservative on stack underflow and method loads.
The underflow path says “don’t optimize this block,” but the loop continues, so the analysis still runs and may convert unsafely. Also,LOAD_ATTRwith method-flag pushes two values; treating it as(1,1)undercounts pushes and can mis-markLOAD_FASTas fully consumed. Consider usingAnyInstruction::stack_effectfor pseudo ops and bailing out on underflow, plus a preciseLOAD_ATTRcase.🔧 Suggested change
- for (i, info) in block.instructions.iter().enumerate() { - let Some(instr) = info.instr.real() else { - continue; - }; - - // Get stack effect - let effect = instr.stack_effect(info.arg); + let mut valid_block = true; + for (i, info) in block.instructions.iter().enumerate() { + let instr_real = info.instr.real(); + + // Get stack effect (includes pseudo ops) + let effect = info.instr.stack_effect(info.arg); let num_popped = if effect < 0 { (-effect) as usize } else { 0 }; let num_pushed = if effect > 0 { effect as usize } else { 0 }; // More precise: calculate actual pops and pushes // For most instructions: pops = max(0, -effect), pushes = max(0, effect) // But some instructions have both pops and pushes - let (pops, pushes) = match instr { + let (pops, pushes) = match instr_real { + Some(Instruction::LoadAttr { idx }) => { + let (_, is_method) = bytecode::decode_load_attr_arg(idx.get(info.arg)); + if is_method { (1, 2) } else { (1, 1) } + } // Instructions that both pop and push - Instruction::BinaryOp { .. } => (2, 1), - Instruction::CompareOp { .. } => (2, 1), - Instruction::ContainsOp(_) => (2, 1), - Instruction::IsOp(_) => (2, 1), - Instruction::UnaryInvert - | Instruction::UnaryNegative - | Instruction::UnaryNot - | Instruction::ToBool => (1, 1), - Instruction::GetIter | Instruction::GetAIter => (1, 1), - Instruction::LoadAttr { .. } => (1, 1), // simplified - Instruction::Call { nargs } => (nargs.get(info.arg) as usize + 2, 1), - Instruction::CallKw { nargs } => (nargs.get(info.arg) as usize + 3, 1), + Some(Instruction::BinaryOp { .. }) => (2, 1), + Some(Instruction::CompareOp { .. }) => (2, 1), + Some(Instruction::ContainsOp(_)) => (2, 1), + Some(Instruction::IsOp(_)) => (2, 1), + Some(Instruction::UnaryInvert) + | Some(Instruction::UnaryNegative) + | Some(Instruction::UnaryNot) + | Some(Instruction::ToBool) => (1, 1), + Some(Instruction::GetIter) | Some(Instruction::GetAIter) => (1, 1), + Some(Instruction::Call { nargs }) => (nargs.get(info.arg) as usize + 2, 1), + Some(Instruction::CallKw { nargs }) => (nargs.get(info.arg) as usize + 3, 1), // Use stack effect for others _ => (num_popped, num_pushed), }; // Pop values from stack for _ in 0..pops { if stack.pop().is_none() { // Stack underflow in simulation - block receives values from elsewhere // Conservative: don't optimize this block - break; + valid_block = false; + break; } } + if !valid_block { + break; + } // Push values to stack with source instruction index - let source = match instr { - Instruction::LoadFast(_) | Instruction::LoadFastLoadFast { .. } => i, + let source = match instr_real { + Some(Instruction::LoadFast(_)) + | Some(Instruction::LoadFastLoadFast { .. }) => i, _ => NOT_LOCAL, }; for _ in 0..pushes { stack.push(source); } } + if !valid_block { + continue; + } // Mark instructions whose values remain on stack at block end for &src in &stack { if src != NOT_LOCAL { unconsumed[src] = true; } }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/codegen/src/ir.rs`:
- Around line 731-747: The current special-casing in the match over instr (the
Instruction enum) is incomplete and can misinterpret the stack delta because
num_popped/num_pushed fallback is ambiguous; update the logic in this match to
be conservative: either enumerate and correctly handle all Instruction variants
with non-trivial pop/push behavior (e.g., BinaryOpInplaceAddUnicode,
LoadFromDictOrDeref, LoadFromDictOrGlobals, Swap, Copy, ForIter, all Build*
instructions) by returning explicit (pops, pushes) tuples, or change the default
branch to return (0, 0) for unknown/complex instructions to avoid incorrect
assumptions; adjust the Call/CallKw handling to still use nargs.get(info.arg)
and keep using info.arg where needed, and add a short comment above the match
noting the conservative assumption so future readers of ir.rs and the match over
instr know why unknown cases return (0,0).
In `@extra_tests/snippets/stdlib_io.py`:
- Around line 17-21: The assertions inside the FileIO context are checking the
wrong variable: change the three assertions that reference result to use the
local variable res (read via FileIO.read()) so they validate the bytes returned
by fio.read(); update the checks near the FileIO block (the assertions currently
using result) to assert len(res) <= 16 * 1024, len(res) >= 0, and
isinstance(res, bytes) respectively.
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
1341-1391: Consider extracting common error-handling logic.The
LoadFastBorrowandLoadFastBorrowLoadFastBorrowhandlers duplicate the unbound-local error creation pattern fromLoadFastandLoadFastLoadFast. While the TODO comments indicate future divergence for the actual load semantics, the error-handling logic will likely remain the same.You could extract a helper to reduce duplication:
fn unbound_local_error(varname: &'static PyStrInterned, vm: &VirtualMachine) -> PyBaseExceptionRef { vm.new_exception_msg( vm.ctx.exceptions.unbound_local_error.to_owned(), format!("local variable '{varname}' referenced before assignment"), ) }This is optional since the current implementation is correct and the duplication may be intentional for future optimization work.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/jit/src/instructions.rs (1)
581-589: HandleLoadFastBorrowLoadFastBorrowin the JIT.With the new borrow optimization, this opcode can be emitted and will currently fall through to
NotSupported, disabling JIT for otherwise valid bytecode. Consider mirroring theLOAD_FAST_LOAD_FASTbehavior here.🛠️ Proposed fix
Instruction::LoadFast(idx) | Instruction::LoadFastBorrow(idx) => { let local = self.variables[idx.get(arg) as usize] .as_ref() .ok_or(JitCompileError::BadBytecode)?; self.stack.push(JitValue::from_type_and_value( local.ty.clone(), self.builder.use_var(local.var), )); Ok(()) } + Instruction::LoadFastBorrowLoadFastBorrow { arg: packed } => { + let oparg = packed.get(arg); + let idx1 = oparg >> 4; + let idx2 = oparg & 0xF; + for idx in [idx1, idx2] { + let local = self.variables[idx as usize] + .as_ref() + .ok_or(JitCompileError::BadBytecode)?; + self.stack.push(JitValue::from_type_and_value( + local.ty.clone(), + self.builder.use_var(local.var), + )); + } + Ok(()) + }
🤖 Fix all issues with AI agents
In `@crates/codegen/src/ir.rs`:
- Around line 758-760: The stack-effect mapping for Instruction::LoadAttr is
wrong for method-form attribute access: update the match arm handling
Instruction::LoadAttr in the stack-effect calculation in ir.rs so that it
returns (1, 2) when the LoadAttr instance has its method flag set (pop 1, push
method+self_or_null = 2) and remains (1, 1) otherwise; locate the match on
Instruction::LoadAttr and branch on the method flag to adjust the push count to
avoid underestimating the simulated stack and unsafe LOAD_FAST_BORROW
conversions.
🧹 Nitpick comments (1)
.cspell.dict/rust-more.txt (1)
92-93:bitvec/Bitvecentries break alphabetical ordering.The dictionary appears to be alphabetically sorted, but
bitvecandBitvecare appended at the end afterwinsock. They should be placed betweenbitorandbstrto maintain consistency.📋 Suggested placement
bitflags bitor +bitvec +Bitvec bitxor bstrAnd remove from end of file:
winresource winsock -bitvec -Bitvec
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_peepholer.py (TODO: 32) dependencies: dependent tests: (no tests depend on peepholer) Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/compiler-core/src/bytecode/instruction.rs (1)
26-55: UpdateBinaryOpInplaceAddUnicodestack effect to -1.The instruction pops 2 values and pushes 1 result (matching the codegen grouping at line 738-742 of
crates/codegen/src/ir.rs), butInstructionMetadata::stack_effectat line 608 still returns 0. This mismatch can skew stack-depth analysis and other compiler passes.🛠️ Proposed fix
- Self::BinaryOpInplaceAddUnicode => 0, + Self::BinaryOpInplaceAddUnicode => -1,
🤖 Fix all issues with AI agents
In `@crates/codegen/src/ir.rs`:
- Around line 724-727: The borrow-analysis in optimize_load_fast_borrow
currently ignores pseudo-ops by using info.instr.real() and continuing, which
can mis-model the stack (pseudo ops like LoadClosure/StoreFastMaybeNull affect
push/pop); change the logic to conservatively bail out for the whole block when
a pseudo-op is encountered: inside the loop over block.instructions detect when
info.instr.real() is None and skip further processing of that block (e.g., set a
flag and break/continue to the next block) so the pass does not make unsafe
assumptions about LOAD_FAST liveness, or alternatively move
optimize_load_fast_borrow to run after pseudo lowering if feasible.
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
1341-1391: Consider tracking the borrow-optimization TODO.If this is intentionally a placeholder, linking it to an issue will make the follow‑up easier. I can help draft the issue or a concrete implementation plan if you want.
| for (i, info) in block.instructions.iter().enumerate() { | ||
| let Some(instr) = info.instr.real() else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Handle pseudo-instructions in the borrow analysis.
optimize_load_fast_borrow runs before pseudo lowering, but info.instr.real() skips pseudo ops (e.g., LoadClosure, StoreFastMaybeNull) that can push/pop. That can mis-model the stack and mark a LOAD_FAST as consumed when it actually survives to block end, which would make a future borrow implementation unsafe. Consider bailing out on pseudo ops or moving this pass after pseudo conversion.
🛡️ Conservative fix (skip block on pseudo ops)
- let Some(instr) = info.instr.real() else {
- continue;
- };
+ let Some(instr) = info.instr.real() else {
+ // Pseudo ops can change stack depth; skip this block to stay conservative.
+ underflow = true;
+ break;
+ };🤖 Prompt for AI Agents
In `@crates/codegen/src/ir.rs` around lines 724 - 727, The borrow-analysis in
optimize_load_fast_borrow currently ignores pseudo-ops by using
info.instr.real() and continuing, which can mis-model the stack (pseudo ops like
LoadClosure/StoreFastMaybeNull affect push/pop); change the logic to
conservatively bail out for the whole block when a pseudo-op is encountered:
inside the loop over block.instructions detect when info.instr.real() is None
and skip further processing of that block (e.g., set a flag and break/continue
to the next block) so the pass does not make unsafe assumptions about LOAD_FAST
liveness, or alternatively move optimize_load_fast_borrow to run after pseudo
lowering if feasible.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.