Skip to content

AnyInstruction helpers + doc imrovments#7757

Merged
youknowone merged 9 commits into
RustPython:mainfrom
ShaharNaveh:any-instr-helpers
May 2, 2026
Merged

AnyInstruction helpers + doc imrovments#7757
youknowone merged 9 commits into
RustPython:mainfrom
ShaharNaveh:any-instr-helpers

Conversation

@ShaharNaveh
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh commented May 1, 2026

Summary by CodeRabbit

  • Refactor
    • Internal instruction/opcode handling was reworked for clearer, more compile-time-checked matching and safer helpers.
    • Several utilities were made compile-time-friendly and some signatures tightened for more efficient internal use.
    • No user-facing behavior or public API changes; functionality and outputs remain unchanged.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 1, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors bytecode/IR handling to use opcode-based APIs (real_opcode/pseudo_opcode) and by-value const fn helpers; updates codegen IR folding, peephole, swap/borrow/deopt, and CFG utilities to match/rewrite on Opcode/PseudoOpcode values and emit Opcode::... .into()/pseudo opcode replacements.

Changes

Cohort / File(s) Summary
Bytecode instruction helpers
crates/compiler-core/src/bytecode/instruction.rs
Added pub const fn real_opcode(self) -> Option<Opcode> and pub const fn pseudo_opcode(self) -> Option<PseudoOpcode>. Converted PseudoInstruction::is_block_push, AnyInstruction::is_pop_block, and AnyInstruction::is_block_push to const fn taking self by value and updated implementations to use opcode matching.
Codegen IR / peephole & CFG
crates/codegen/src/ir.rs
Replaced pattern-matching on constructed Instruction variants with opcode-based matching via instr.real_opcode() / instr.pseudo_opcode(). Updated constant folding, peephole rewrites, superinstruction emission, jump/branch folding (emit PseudoOpcode::Jump), borrow optimizations/deopts, swap/static-reordering helpers (is_swappable/stores_to) and related CFG/exception cleanup to operate on opcode values and emit Opcode::X.into(). Adjusted helper signatures and usages accordingly.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • coolreader18
  • youknowone

Poem

🐰
I hop through opcodes, quiet and spry,
Folding bytes beneath a starlit sky.
Swaps reset, old patterns unwind,
Tiny paws leave tidy trails behind. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions 'doc improvements' but the actual changes are about refactoring instruction matching from variants to opcodes, with minor API additions. The title is misleading about the primary scope of changes. Update the title to accurately reflect the main change, such as 'Refactor instruction matching to use opcode-based checks' or 'Use opcode-based matching in instruction optimizer'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 97.53% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 4291-4296: The match arm for Opcode::LoadFastBorrowLoadFastBorrow
incorrectly uses a destructuring pattern with `{ .. }` on a unit variant; update
the pattern in the match over info.instr.real_opcode() to use the unit variant
form Some(Opcode::LoadFastBorrowLoadFastBorrow) => { ... } (leaving the body
that sets info.instr = Opcode::LoadFastLoadFast.into() unchanged) so it compiles
like the other Opcode matches.
🪄 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: 9f981275-9e79-44e4-89fd-a67efd45c11d

📥 Commits

Reviewing files that changed from the base of the PR and between b20ad3d and 5cecfc4.

📒 Files selected for processing (1)
  • crates/codegen/src/ir.rs

Comment thread crates/codegen/src/ir.rs Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)

3074-3080: ⚡ Quick win

Factor out the repeated borrow-deopt remap.

The same LoadFastBorrow* -> LoadFast* rewrite is duplicated across several deopt passes. A small shared helper would make this opcode migration much harder to drift the next time borrow variants change.

♻️ Suggested cleanup
+fn deoptimize_borrowed_fast(info: &mut InstructionInfo) -> bool {
+    match info.instr.real_opcode() {
+        Some(Opcode::LoadFastBorrow) => {
+            info.instr = Opcode::LoadFast.into();
+        }
+        Some(Opcode::LoadFastBorrowLoadFastBorrow) => {
+            info.instr = Opcode::LoadFastLoadFast.into();
+        }
+        _ => return false,
+    }
+    true
+}
+
         fn deoptimize_block_borrows(block: &mut Block) {
             for info in &mut block.instructions {
-                match info.instr.real_opcode() {
-                    Some(Opcode::LoadFastBorrow) => {
-                        info.instr = Opcode::LoadFast.into();
-                    }
-                    Some(Opcode::LoadFastBorrowLoadFastBorrow) => {
-                        info.instr = Opcode::LoadFastLoadFast.into();
-                    }
-                    _ => {}
-                }
+                let _ = deoptimize_borrowed_fast(info);
             }
         }

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

Also applies to: 3254-3260, 3515-3521, 3669-3675, 3722-3727, 3829-3835, 4246-4253, 4569-4577

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/codegen/src/ir.rs` around lines 3074 - 3080, Extract a small helper
(e.g. fn remap_borrow_variants(op: Opcode) -> Option<Opcode>) that maps borrow
forms to their non-borrow equivalents (handle Opcode::LoadFastBorrow ->
Opcode::LoadFast and Opcode::LoadFastBorrowLoadFastBorrow ->
Opcode::LoadFastLoadFast, etc.), then replace the duplicated match blocks by
calling this helper: call info.instr.real_opcode(), pass it into
remap_borrow_variants, and if Some(mapped) set info.instr = mapped.into(); apply
the same helper usage at the other duplicated locations that reference
Opcode::LoadFastBorrow, Opcode::LoadFastBorrowLoadFastBorrow,
Opcode::LoadFastLoadFast and info.instr to avoid drifting copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 3074-3080: Extract a small helper (e.g. fn
remap_borrow_variants(op: Opcode) -> Option<Opcode>) that maps borrow forms to
their non-borrow equivalents (handle Opcode::LoadFastBorrow -> Opcode::LoadFast
and Opcode::LoadFastBorrowLoadFastBorrow -> Opcode::LoadFastLoadFast, etc.),
then replace the duplicated match blocks by calling this helper: call
info.instr.real_opcode(), pass it into remap_borrow_variants, and if
Some(mapped) set info.instr = mapped.into(); apply the same helper usage at the
other duplicated locations that reference Opcode::LoadFastBorrow,
Opcode::LoadFastBorrowLoadFastBorrow, Opcode::LoadFastLoadFast and info.instr to
avoid drifting copies.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: fe5d138f-eb5b-4a8a-b53b-e6e107c5fdc0

📥 Commits

Reviewing files that changed from the base of the PR and between 5cecfc4 and 9f1593c.

📒 Files selected for processing (1)
  • crates/codegen/src/ir.rs

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)

3074-3081: ⚡ Quick win

Extract shared borrow-deopt rewrite into one helper

The same LoadFastBorrow* -> LoadFast* rewrite logic is duplicated across multiple passes. Please centralize this into a small helper to avoid future drift.

Proposed refactor sketch
+fn deoptimize_borrow_instr(info: &mut InstructionInfo) {
+    info.instr = match info.instr.real_opcode() {
+        Some(Opcode::LoadFastBorrow) => Opcode::LoadFast,
+        Some(Opcode::LoadFastBorrowLoadFastBorrow) => Opcode::LoadFastLoadFast,
+        _ => return,
+    }
+    .into();
+}
...
-            for info in &mut block.instructions {
-                match info.instr.real_opcode() {
-                    Some(Opcode::LoadFastBorrow) => {
-                        info.instr = Opcode::LoadFast.into();
-                    }
-                    Some(Opcode::LoadFastBorrowLoadFastBorrow) => {
-                        info.instr = Opcode::LoadFastLoadFast.into();
-                    }
-                    _ => {}
-                }
-            }
+            for info in &mut block.instructions {
+                deoptimize_borrow_instr(info);
+            }

As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

Also applies to: 3254-3261, 3515-3522, 3669-3676

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/codegen/src/ir.rs` around lines 3074 - 3081, Extract the repeated
"borrow-deopt" rewrite into a small helper (e.g., rewrite_loadfast_borrow) and
call it from each pass instead of duplicating the match; the helper should take
a mutable reference to the instruction container used in these passes (the same
type as `info`), pattern-match `info.instr.real_opcode()` and map
`Opcode::LoadFastBorrow` -> `Opcode::LoadFast` and
`Opcode::LoadFastBorrowLoadFastBorrow` -> `Opcode::LoadFastLoadFast`, updating
`info.instr` accordingly. Replace the duplicated match blocks currently in the
functions that contain the shown snippet and the similar blocks at the other
locations (around the code handling 3254-3261, 3515-3522, 3669-3676) with a
single call to this helper to centralize the logic and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 3074-3081: Extract the repeated "borrow-deopt" rewrite into a
small helper (e.g., rewrite_loadfast_borrow) and call it from each pass instead
of duplicating the match; the helper should take a mutable reference to the
instruction container used in these passes (the same type as `info`),
pattern-match `info.instr.real_opcode()` and map `Opcode::LoadFastBorrow` ->
`Opcode::LoadFast` and `Opcode::LoadFastBorrowLoadFastBorrow` ->
`Opcode::LoadFastLoadFast`, updating `info.instr` accordingly. Replace the
duplicated match blocks currently in the functions that contain the shown
snippet and the similar blocks at the other locations (around the code handling
3254-3261, 3515-3522, 3669-3676) with a single call to this helper to centralize
the logic and avoid drift.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 8be3551c-0ae3-47fd-9d76-a8db7fa12480

📥 Commits

Reviewing files that changed from the base of the PR and between 9f1593c and d0b52d0.

📒 Files selected for processing (1)
  • crates/codegen/src/ir.rs

@youknowone youknowone merged commit f2ad84a into RustPython:main May 2, 2026
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants