Align bytecode CFG cleanup with CPython#7781
Open
youknowone wants to merge 72 commits intoRustPython:mainfrom
Open
Align bytecode CFG cleanup with CPython#7781youknowone wants to merge 72 commits intoRustPython:mainfrom
youknowone wants to merge 72 commits intoRustPython:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThese changes refactor the code generator's control-flow infrastructure and bytecode optimization passes. Key updates include conditional constant folding, simplified try/except end paths, loop prefix pre-evaluation, and expanded IR optimization with layout-aware CFG analysis and short-circuit target threading. Symbol table analysis is unified through a centralized scope-type helper. ChangesCodegen Control-Flow and IR Optimization Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_compile.py (TODO: 16) dependencies: dependent tests: (no tests depend on compile) Legend:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/codegen/src/symboltable.rs (1)
1061-1070: 💤 Low valueOptional: document the contrast with
compile.rs::is_function_like.This
is_function_like_scope(Function/AsyncFunction/Lambda/Comprehension/Annotation) intentionally differs from theis_function_likepredicate incompile.rs(Function/AsyncFunction/Annotation/TypeParams). The set used here matches CPython'sanalyze_cellssemantics insymtable.c, while the codegen variant guardsCodeFlags. A short doc comment would help future readers avoid conflating them.📝 Suggested doc comment
+ /// Scopes that participate in CPython's `analyze_cells` step (LOCAL→CELL + /// promotion and free-var removal). This intentionally differs from + /// `compile.rs::is_function_like`, which controls `CodeFlags` and uses a + /// different set (e.g. includes `TypeParams`, excludes `Lambda`/`Comprehension`). fn is_function_like_scope(typ: CompilerScope) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/symboltable.rs` around lines 1061 - 1070, Add a short doc comment above is_function_like_scope explaining that its set (Function, AsyncFunction, Lambda, Comprehension, Annotation) intentionally differs from the predicate compile.rs::is_function_like (which uses Function, AsyncFunction, Annotation, TypeParams); note that is_function_like_scope mirrors CPython's analyze_cells semantics in symtable.c while the compile.rs variant is used for guarding CodeFlags, so readers should not conflate the two. Include references to both symbols (is_function_like_scope and compile.rs::is_function_like) in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 9229-9233: fast_scan_many_locals currently treats every local
index >=64 as unknown, causing parameters beyond index 63 to be misclassified;
pass the function's parameter count (nparams) into fast_scan_many_locals (or
otherwise make the helper aware of nparams) and mark any local index < nparams
as initialized/parameter so they are not converted to LOAD_FAST_CHECK or split
by LOAD_FAST_LOAD_FAST; update the call sites (and the fast path logic inside
fast_scan_many_locals and its use of merged_cell_local) to exempt indices <
nparams from the unknown high-index handling (also apply the same change to the
related code block around lines 9344-9402 that duplicates this fast path).
- Around line 10627-10672: The helpers opposite_short_circuit_target and
same_short_circuit_target must verify the entire short-circuit stub shape, not
just the first two ops and the trailing conditional jump; modify both functions
to (1) ensure the first instruction is exactly Instruction::Copy with
i.get(first.arg) == 1 and the second is exactly Instruction::ToBool, (2) iterate
over the instructions between index 2 and cond_idx (exclusive) and require each
instruction's real() to be either None or one of the allowed marker ops (e.g.
Instruction::Nop or Instruction::NotTaken) — reject the block if any other real
instruction appears, and (3) only then perform the existing matches against
block.instructions[cond_idx].instr.real(); apply the same stricter validation in
both opposite_short_circuit_target and same_short_circuit_target so
jump_threading_impl cannot skip real instructions when threading.
---
Nitpick comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 1061-1070: Add a short doc comment above is_function_like_scope
explaining that its set (Function, AsyncFunction, Lambda, Comprehension,
Annotation) intentionally differs from the predicate
compile.rs::is_function_like (which uses Function, AsyncFunction, Annotation,
TypeParams); note that is_function_like_scope mirrors CPython's analyze_cells
semantics in symtable.c while the compile.rs variant is used for guarding
CodeFlags, so readers should not conflate the two. Include references to both
symbols (is_function_like_scope and compile.rs::is_function_like) in the comment
for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ce25f7be-2b5c-4295-b7ae-f9c2f4a2465b
⛔ Files ignored due to path filters (1)
Lib/test/test_peepholer.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 4648-4680: The current block_has_simple_scope_exit() treats
certain already-lowered fast-local superinstructions as non-simple and thus
causes leading_bool_guard_has_scope_exit_successor() to prematurely stop
segments; update the match arm that currently allows LoadFastLoadFast,
LoadFastBorrowLoadFastBorrow, etc., to also accept the lowered opcodes
StoreFastLoadFast, StoreFastStoreFast and LoadFastCheck (i.e., add
Instruction::StoreFastLoadFast { .. }, Instruction::StoreFastStoreFast { .. },
and Instruction::LoadFastCheck { .. } to the permissive Some(...) pattern) so
those successors are considered simple scope-exit blocks and the borrow deopt
path is not skipped.
- Around line 2655-2671: The check that sets
preserve_block_start_no_location_nop runs before the trailing conditional jump
is converted to an unconditional jump, so patterns like "LOAD_CONST True;
POP_JUMP_IF_TRUE <self>" are not recognized; move or re-run the
preserves_pure_self_loop_anchor detection after the jump-rewrite phase (i.e.,
after the code that turns the trailing conditional into a plain jump), or adjust
the predicate to treat the specific conditional that will be rewritten as
unconditional (use info.instr / info.instr.real() and is_unconditional_jump or
detect POP_JUMP_IF_TRUE with a constant true target) so that set_to_nop and the
preserves_pure_self_loop_anchor logic see the post-rewrite form before deciding
to set preserve_block_start_no_location_nop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6339f5f3-9b75-4593-a192-ec6e1323018f
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/codegen/src/compile.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/codegen/src/ir.rs (2)
4648-4663:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat already-lowered fast-local opcodes as simple scope exits.
This helper still rejects
LoadFastCheck,StoreFastLoadFast, andStoreFastStoreFast, even though this pass runs after those opcodes can already exist. That makesleading_bool_guard_has_scope_exit_successor()stop on structurally simple cleanup successors and skip the intended borrow deopt.Suggested fix
Some( Instruction::Nop | Instruction::NotTaken | Instruction::LoadConst { .. } | Instruction::LoadSmallInt { .. } | Instruction::StoreFast { .. } + | Instruction::StoreFastLoadFast { .. } + | Instruction::StoreFastStoreFast { .. } | Instruction::StoreName { .. } | Instruction::LoadFast { .. } | Instruction::LoadFastBorrow { .. } + | Instruction::LoadFastCheck { .. } | Instruction::LoadFastLoadFast { .. } | Instruction::LoadFastBorrowLoadFastBorrow { .. } | Instruction::BuildTuple { .. }, ) => {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/ir.rs` around lines 4648 - 4663, The helper block_has_simple_scope_exit currently treats many opcodes as simple exits but still rejects lowered fast-local opcodes; update the match in block_has_simple_scope_exit (the loop over block.instructions) to also consider LoadFastCheck, StoreFastLoadFast, and StoreFastStoreFast as simple scope exits (i.e., include them in the same Some(Instruction::...) arm that returns true) so that leading_bool_guard_has_scope_exit_successor will recognize structurally simple cleanup successors after lowering; ensure you only add those opcode variants to the existing accepted list and keep the early return behavior unchanged.
2655-2679:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the self-loop anchor after rewriting the conditional.
preserves_pure_self_loop_anchoris evaluated before Line 2673 rewritesPOP_JUMP_* <self>into a plain jump. A block likeLOAD_CONST True; POP_JUMP_IF_TRUE <self>still looks non-unconditional here, so the block-start NOP is not preserved and gets stripped later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/ir.rs` around lines 2655 - 2679, The code computes preserves_pure_self_loop_anchor before rewriting the conditional at block.instructions[i+1], so POP_JUMP_IF_* <self> isn’t treated as unconditional and the NOP gets dropped; fix by computing or re-checking the self-loop anchor after the rewrite (i.e., move or recompute the preserves_pure_self_loop_anchor logic to run after the branch that sets block.instructions[i+1] to an unconditional Jump) and then set block.instructions[i].preserve_block_start_no_location_nop = true when the recalculated condition holds; reference set_to_nop, preserves_pure_self_loop_anchor, block.instructions, and preserve_block_start_no_location_nop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 611-619: The predicate statements_end_with_with_cleanup_scope_exit
only matches ast::Stmt::With and therefore misses nested async-with nodes;
update its match arm to handle ast::Stmt::AsyncWith (likely
ast::Stmt::AsyncWith(ast::StmtAsyncWith { body, .. })) the same way as
ast::Stmt::With so it recurses into async with bodies and calls
Self::statements_end_with_scope_exit and
Self::statements_end_with_with_cleanup_scope_exit for that body as well,
ensuring nested async with cleanup scopes are detected by
statements_end_with_with_cleanup_scope_exit and preventing
preserve_outer_cleanup_target_nop from dropping needed NOPs.
- Around line 19608-19616: The test function
test_loop_not_in_conditional_body_threads_true_path_to_jump_back currently uses
the expression "not k in 'bBhHiIlLfd'" which parses as unary not applied to the
in comparison; update the test string passed to compile_exec inside that
function to use the dedicated NotIn operator by replacing "not k in
'bBhHiIlLfd'" with "k not in 'bBhHiIlLfd'" so the test actually exercises the
NotIn code path (locate the test by the function name and the compile_exec call
with the multi-line string).
In `@crates/codegen/src/ir.rs`:
- Around line 4672-4681: The helper leading_bool_guard_has_scope_exit_successor
currently inspects block.next directly and can be misled by empty layout blocks;
change the successor check to skip empty placeholders by normalizing each
successor with next_nonempty_block(blocks, successor) before testing for scope
exits. In practice, in leading_bool_guard_has_scope_exit_successor (and while
using trailing_conditional_jump_index and block_has_simple_scope_exit), map both
block.instructions[cond_idx].target and block.next through
next_nonempty_block(blocks, ...) and then perform the NULL check and
block_has_simple_scope_exit check on the normalized successor.
---
Duplicate comments:
In `@crates/codegen/src/ir.rs`:
- Around line 4648-4663: The helper block_has_simple_scope_exit currently treats
many opcodes as simple exits but still rejects lowered fast-local opcodes;
update the match in block_has_simple_scope_exit (the loop over
block.instructions) to also consider LoadFastCheck, StoreFastLoadFast, and
StoreFastStoreFast as simple scope exits (i.e., include them in the same
Some(Instruction::...) arm that returns true) so that
leading_bool_guard_has_scope_exit_successor will recognize structurally simple
cleanup successors after lowering; ensure you only add those opcode variants to
the existing accepted list and keep the early return behavior unchanged.
- Around line 2655-2679: The code computes preserves_pure_self_loop_anchor
before rewriting the conditional at block.instructions[i+1], so POP_JUMP_IF_*
<self> isn’t treated as unconditional and the NOP gets dropped; fix by computing
or re-checking the self-loop anchor after the rewrite (i.e., move or recompute
the preserves_pure_self_loop_anchor logic to run after the branch that sets
block.instructions[i+1] to an unconditional Jump) and then set
block.instructions[i].preserve_block_start_no_location_nop = true when the
recalculated condition holds; reference set_to_nop,
preserves_pure_self_loop_anchor, block.instructions, and
preserve_block_start_no_location_nop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b5a64a8f-10f8-467d-ac25-167695cc1990
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Comment on lines
+19608
to
+19616
| #[test] | ||
| fn test_loop_not_in_conditional_body_threads_true_path_to_jump_back() { | ||
| let code = compile_exec( | ||
| "\ | ||
| def f(native, array): | ||
| for k in native: | ||
| if not k in 'bBhHiIlLfd': | ||
| del array[k] | ||
| ", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python's grammar/AST, how is not x in yparsed compared withx not in y? Does not x in ybecome unarynotapplied to anincomparison rather than thenot in comparison operator?
💡 Result:
In Python's grammar and AST, x not in y is parsed as a single comparison operator NotIn applied to x and y [1][2]. The grammar explicitly defines notin_bitwise_or: 'not' 'in' bitwise_or, producing a Compare node with NotIn operator in the AST [1][2]. In contrast, not x in y is parsed as the unary logical not applied to the Compare (x in y), because unary not has lower precedence than comparisons, and the grammar for not_test is 'not' not_test | comparison [3]. This produces a UnaryOp with Not operator whose operand is a Compare with In [4][5]. Thus, yes, not x in y becomes unary not applied to an in comparison, not the not in operator [6][7]. Both expressions evaluate equivalently (due to special casing or semantics), but their parse trees differ, and style guides prefer x not in y for clarity [8][9].
Citations:
- 1: https://docs.python.org/3/reference/grammar.html
- 2: https://github.com/python/cpython/blob/main/Grammar/python.gram
- 3: https://docs.python.org/3.11/reference/expressions.html
- 4: https://docs.python.org/3.15/library/ast.html
- 5: http://docs.python.org/3/library/ast.html
- 6: https://stackoverflow.com/questions/8738388/x-not-in-y-or-not-x-in-y
- 7: https://stackoverflow.com/questions/2211706/are-not-in-and-is-not-both-operators-if-so-are-they-in-any-way-different-t
- 8: https://fabiangunzinger.github.io/idiomatic-python/
- 9: https://pycodestyle.pycqa.org/en/2.9.0/_modules/pycodestyle.html
Use k not in ... if this test is meant to cover NotIn.
The expression not k in 'bBhHiIlLfd' parses as unary not applied to the in comparison, not the dedicated NotIn operator. Since the test name indicates coverage of the NotIn path, use k not in 'bBhHiIlLfd' instead for accurate test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegen/src/compile.rs` around lines 19608 - 19616, The test function
test_loop_not_in_conditional_body_threads_true_path_to_jump_back currently uses
the expression "not k in 'bBhHiIlLfd'" which parses as unary not applied to the
in comparison; update the test string passed to compile_exec inside that
function to use the dedicated NotIn operator by replacing "not k in
'bBhHiIlLfd'" with "k not in 'bBhHiIlLfd'" so the test actually exercises the
NotIn code path (locate the test by the function name and the compile_exec call
with the multi-line string).
8850486 to
6f0f259
Compare
6f0f259 to
5bb40c9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores