Reduce usage JumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare
#6524
Conversation
📝 WalkthroughWalkthroughThis PR refactors comparison operation compilation in the bytecode generator, introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…p-if-true-false-pop
…p-if-true-false-pop
JumpIfTrueOrPop & JumpIfFalseOrPop opcdesJumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare
|
This started as an attempt to remove |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
4118-4126: Commented-out code and inconsistent refactoring with PR goal.The PR objective is to reduce usage of
JumpIfFalseOrPopopcode, but:
- Lines 4118-4123 contain commented-out code showing the intended replacement pattern
- Line 4125 still uses
JumpIfFalseOrPopThis appears to be an intentional reversion based on commit history ("Reverted PopJumpIfFalse changes for comparisons"), but the commented code creates noise and the inconsistency should be documented. Consider either:
- Removing the commented code if the reversion is permanent
- Adding a TODO/FIXME comment explaining why
JumpIfFalseOrPopis still used here🔎 Clean up commented code
self.compile_addcompare(op); // if comparison result is false, we break with this value; if true, try the next one. - /* - emit!(self, Instruction::CopyItem { index: 1 }); - // emit!(self, Instruction::ToBool); // TODO: Uncomment this - emit!(self, Instruction::PopJumpIfFalse { target: cleanup }); - emit!(self, Instruction::PopTop); - */ - + // NOTE: Using JumpIfFalseOrPop here instead of CopyItem+PopJumpIfFalse+PopTop + // because chained comparisons require the comparison result to remain on stack + // for cleanup when short-circuiting. emit!(self, Instruction::JumpIfFalseOrPop { target: cleanup });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/codegen/src/compile.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/codegen/src/compile.rs
🔇 Additional comments (4)
crates/codegen/src/compile.rs (4)
4049-4066: Clean helper extraction for comparison operations.The
compile_addcomparemethod is a well-structured helper that mapsCmpOpvariants to their corresponding bytecode instructions. This follows the CPython reference and improves code organization.
4467-4491: Correct implementation of the opcode replacement pattern.The refactoring correctly replaces
JumpIfFalseOrPop/JumpIfTrueOrPopwith the explicit sequence:
CopyItem { index: 1 }- duplicates TOSPopJumpIfFalse/PopJumpIfTrue- conditional jump, pops TOSPopTop- cleans up if no jump occurredThis properly implements short-circuit evaluation:
- For
and: if any value is false, jump with that value on stack- For
or: if any value is true, jump with that value on stackThe semantics are preserved while using more explicit opcodes.
4086-4141: The refactoredcompile_comparecorrectly aligns with CPython's chained comparison behavior.The stack manipulation pattern with
Swap { index: 2 }andCopyItem { index: 2 }achieves the same effect as CPython'sDUP_TOPandROT_THREEinstructions. Both approaches preserve the middle operand for the next comparison in the chain. The cleanup block correctly handles early exit by restoring stack invariants when a comparison result is false. The implementation usingJumpIfFalseOrPopmatches CPython's semantics for conditional chaining.
4033-4035: Verify stack cleanup when guard evaluates to false in default case.When the default case guard evaluates to false and jumps to
end(line 4039), the guard result is left on the stack:
- After
CopyItem:[guard_result, guard_result]- After
PopJumpIfFalse(false branch): pops the copy, leaving[guard_result]PopToponly executes on true pathThe
endblock performs no stack cleanup, so verify whether the guard result should be popped in the false path or if this is intentional handling.
| } | ||
| ); | ||
| } | ||
| /* |
There was a problem hiding this comment.
what was the blocker of this?
There was a problem hiding this comment.
That's the only reason why both opcodes stayed:/
This change caused regression for
RustPython/Lib/test/test_compile.py
Lines 1073 to 1081 in 61ddd98
Not because it found an EXTENDED_ARG, but because iterating over dis.Bytecode(...) gave an error
There was a problem hiding this comment.
if that's because our test_compile is an old one, replacing it will be also good.
| } | ||
| ); | ||
| } | ||
| /* |
There was a problem hiding this comment.
if that's because our test_compile is an old one, replacing it will be also good.
Implements python/cpython#102870
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.