instruction improvements#6829
Conversation
📝 WalkthroughWalkthroughThis PR updates bytecode and codegen to align with CPython 3.14: adds/renames instructions (LoadSmallInt, EndFor, PopIter, ReturnGenerator), consolidates Jump/JumpNoInterrupt handling, adjusts async-function scope propagation, and updates VM/JIT and oparg enums to support new intrinsics and loop semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source AST
participant Codegen as Codegen (compile.rs)
participant IR as IR (ir.rs)
participant Emitter as Bytecode Emitter (compiler-core)
participant VM as VM / JIT
Source->>Codegen: traverse AST (FunctionDef / async)
Codegen->>IR: emit pseudo-instructions (Jump / JumpNoInterrupt, END_FOR markers, LoadSmallInt hints)
IR->>Emitter: map pseudo → real instructions (JumpBackwardNoInterrupt, EndFor, PopIter, LoadSmallInt)
Emitter->>VM: write bytecode with new opcodes and intrinsics
VM->>VM: execute patterns (END_FOR/POP_ITER, ReturnGenerator, StopIterationError, AsyncGenWrap)
VM->>JIT: JIT handles LoadSmallInt as immediate const
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Click to expand dependency informationLegend:
|
Sorry, something went wrong.
12c443f to
9b1d6d6
Compare
January 21, 2026 15:19
There was a problem hiding this comment.
Actionable comments posted: 2
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/symboltable.rs (1)
2096-2172: Ensure nested-scope detection treatsAsyncFunctionas a function.Now that
AsyncFunctionis a distinct scope,enter_scopestill only treatsCompilerScope::Functionas “nested.” That will misclassify nested defs inside async functions. Please update the nesting predicate.🐛 Suggested fix in
enter_scope- .map(|table| table.is_nested || table.typ == CompilerScope::Function) + .map(|table| { + table.is_nested + || matches!(table.typ, CompilerScope::Function | CompilerScope::AsyncFunction) + })
🤖 Fix all issues with AI agents
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 509-514: The ForIter arm in Instruction::stack_effect currently
ignores the jump parameter and always returns 1; update the ForIter handling in
crates/compiler-core/src/bytecode/instruction.rs (the ForIter match arm inside
the stack_effect implementation for Instruction) to return 1 when jump is false
and 0 when jump is true so it matches CPython 3.14 semantics (jump=False => +1,
jump=True => 0); ensure the change preserves the existing comment/context about
CPython behavior and does not affect other Instruction arms.
In `@crates/vm/src/frame.rs`:
- Around line 2724-2746: The StopIteration handling in the match arm for
bytecode::IntrinsicFunction1::StopIterationError currently uses
arg.class().is(vm.ctx.exceptions.stop_iteration) which only matches the exact
StopIteration type; change this to use
arg.fast_isinstance(vm.ctx.exceptions.stop_iteration) so subclasses of
StopIteration are also treated as StopIteration, keeping the same behavior for
converting to vm.new_runtime_error("coroutine raised StopIteration") and
preserving the existing else branch that re-raises via arg.downcast().
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2313-2327: Guard the END_FOR skip with a POP_ITER check.Skipping to
target + 1assumes a valid POP_ITER follows END_FOR. Consider guarding to avoid out-of-bounds or unexpected bytecode layouts.💡 Suggested defensive guard
- let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) { - if matches!(unit.op, bytecode::Instruction::EndFor) { - // Skip END_FOR, jump to next instruction - bytecode::Label(target.0 + 1) - } else { - // Legacy pattern: jump directly to target (POP_TOP/POP_ITER) - target - } - } else { - target - }; + let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) { + if matches!(unit.op, bytecode::Instruction::EndFor) + && matches!( + self.code.instructions.get(target_idx + 1).map(|u| &u.op), + Some(bytecode::Instruction::PopIter) + ) + { + // Skip END_FOR, jump to POP_ITER + bytecode::Label(target.0 + 1) + } else { + // Legacy pattern: jump directly to target (POP_TOP/POP_ITER) + target + } + } else { + target + };
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 7763-7772: The small-int fast-path currently only accepts 0..=255
via value.to_u8(); change it to accept the CPython 3.14 small-int cache range
-5..=256 by converting the integer to a signed type (e.g. value.to_i64()) and
checking if it falls within -5..=256, then use that signed small_int for the
emitted argument; adjust the cast passed to self.emit_arg to convert the signed
small_int into the parameter type expected by emit_arg/Instruction::LoadSmallInt
(e.g. compute small_int as i32 and cast to u32 when calling emit_arg, and update
Instruction::LoadSmallInt idx type if necessary) so negative values and 256 are
handled correctly while still adding the constant via
self.arg_constant(ConstantData::Integer { .. }).
- Around line 4989-4993: In compile_break_continue and unwind_fblock, replace
the existing POP_TOP-based loop cleanup with the CPython 3.14 END_FOR + POP_ITER
pattern: instead of emitting Instruction::PopTop for for-loop teardown paths,
emit Instruction::EndFor followed by Instruction::PopIter so the VM's
special-skip behavior (skip END_FOR to reach POP_ITER) is preserved; update all
break/continue/return cleanup sites within those functions to use this pattern
and ensure any comments reflect the CPython 3.14 semantics.
♻️ Duplicate comments (1)
crates/compiler-core/src/bytecode/instruction.rs (1)
447-447: Simplified stack_effect signature removes jump parameter.The
stack_effectmethod now takes onlyarg: OpArg, removing thejump: boolparameter. This simplification is consistent across the codebase.However, note that
ForIternow returns a fixed+1(line 509), which differs from CPython's behavior wherejump=True(iterator exhausted) should return0andjump=False(value produced) should return+1. This was flagged in a previous review.Since the
ir.rsstack depth analysis no longer distinguishes jump paths, this may cause conservative stack depth estimates but shouldn't cause underflow. The comment on the previous review explains the semantic difference.Also applies to: 509-509
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2313-2332: Runtime instruction inspection may indicate a compiler-level opportunity.The code inspects the instruction stream at runtime to detect the CPython 3.14
END_FOR/POP_ITERpattern and skipEND_FORon loop exhaustion. While this works correctly, runtime instruction inspection is unusual and could be fragile if bytecode patterns change.Consider whether the compiler could emit a different jump target directly, or if
FOR_ITERshould have an additional operand to indicate the number of instructions to skip. This would eliminate the runtime pattern matching.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 7760-7770: The JIT path mis-decodes LoadSmallInt by zero-extending
the stored u32 when computing small_int (currently doing idx.get(arg) as i64);
update the JIT handler that decodes LoadSmallInt (the place that assigns
small_int from idx.get(arg)) to cast to i32 first and then to i64 (i.e.,
small_int = (idx.get(arg) as i32) as i64) so negative values preserve
two's-complement sign like the VM handler (which uses idx.get(arg) as i32);
ensure any comments/reference to LoadSmallInt, emit_arg, and arg_constant remain
consistent.
♻️ Duplicate comments (1)
crates/compiler-core/src/bytecode/instruction.rs (1)
447-728: Re-check FOR_ITER jump-path stack effect with the new API.
stack_effectnow ignores jump direction andForIterreturns+1. If jump-path depth still differs (iterator exhausted), stack depth analysis can drift. Consider a jump-aware path or special-casingForIterin depth analysis if needed.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
339-346: Add a debug assertion that JumpNoInterrupt is backward-only.This arm always emits
JumpBackwardNoInterrupt; an assertion would catch unexpected forward targets early.♻️ Suggested debug-assert
AnyInstruction::Pseudo(PseudoInstruction::JumpNoInterrupt { .. }) if target != BlockIdx::NULL => { - // JumpNoInterrupt is always backward (used in yield-from/await loops) + let target_offset = block_to_offset[target.idx()].0; + debug_assert!( + target_offset <= current_offset, + "JumpNoInterrupt must be backward" + ); + // JumpNoInterrupt is always backward (used in yield-from/await loops) Instruction::JumpBackwardNoInterrupt { target: Arg::marker(), } }
Sorry, something went wrong.
40a43f3
into
RustPython:main
Jan 22, 2026
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.