Skip to content

Move some free standing functions to ir::InstructionInfo methods - #8209

Merged
youknowone merged 4 commits into
RustPython:mainfrom
ShaharNaveh:ir-cleanup-3
Jul 4, 2026
Merged

Move some free standing functions to ir::InstructionInfo methods#8209
youknowone merged 4 commits into
RustPython:mainfrom
ShaharNaveh:ir-cleanup-3

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved instruction sequencing and line/location tracking to produce more consistent compiled output and avoid edge-case misalignment.
    • Fixed jump-offset resolution and optimization paths to better preserve correct behavior during code generation.
  • Refactor
    • Consolidated and standardized instruction and location-related operations into a unified approach, simplifying downstream code generation and improving maintainability.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b9226173-dee9-4926-a558-14f07fbc3d31

📥 Commits

Reviewing files that changed from the base of the PR and between 5456caf and c25019a.

📒 Files selected for processing (1)
  • crates/codegen/src/ir.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/codegen/src/ir.rs

📝 Walkthrough

Walkthrough

This PR moves instruction sizing, location, predicate, and rewrite helpers onto InstructionInfo, then updates jump resolution, assembly emission, CFG rewrites, constant folding, exception handling, and tests to use the new methods.

Changes

InstructionInfo method migration

Layer / File(s) Summary
Core InstructionInfo methods and debug checks
crates/codegen/src/ir.rs
InstructionInfo gains setters, location helpers, size and linetable computation, nop helpers, predicates, and debug validation, replacing former free helper logic.
Jump sizing and assembly emission
crates/codegen/src/ir.rs
Jump offset resolution and linetable emission switch to InstructionInfo::instr_size() and InstructionInfo::instruction_linetable_location(), with location propagation handled through InstructionInfo methods.
CFG analysis and swap rewrites
crates/codegen/src/ir.rs
Basic-block growth, line-number checks, const-loading detection, swap timing, and load-const optimization switch to InstructionInfo methods such as empty, instruction_lineno, loads_const, nop_out_no_location, stores_to, and maybe_instr_make_load_smallint.
Line propagation and unreachable-block cleanup
crates/codegen/src/ir.rs
CFG duplication and propagation logic uses InstructionInfo::is_jump(), instruction_is_no_location(), instr_set_location(), and instr_location() while redundant NOP and jump removal uses set_to_nop().
Superinstructions, exception marking, and jump threading
crates/codegen/src/ir.rs
Superinstruction fusion, exception-handler marking, warm/cold traversal, jump threading, and pseudo-op conversion use InstructionInfo jump and block-push predicates with nop rewrites.
Tests updated for InstructionInfo API
crates/codegen/src/ir.rs
Tests switch to InstructionInfo::set_to_nop() and InstructionInfo::instruction_lineno() for nop behavior and line-number assertions.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • RustPython/RustPython#7721: Both PRs modify crates/codegen/src/ir.rs around InstructionInfo-driven instruction sizing, linetable/location logic, and NOP cleanup behavior.
  • RustPython/RustPython#7757: Both PRs change instruction-recognition helpers in crates/codegen/src/ir.rs around jump and store-style checks.
  • RustPython/RustPython#7942: Both PRs make substantial code-level changes in crates/codegen/src/ir.rs around instruction metadata and jump sizing/threading logic.

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main refactor of moving free-standing ir helpers onto InstructionInfo methods.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

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)

2682-2713: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the three super-instruction arms into one call.

All three match arms differ only in the super_op value while sharing the split_at_mut(1) + make_super_instruction(...) logic. Extract the differing opcode first, then call once. The first arm's nested if is equivalent to matching (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) directly.

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."

♻️ Proposed refactor
-                match (block.instructions[i].instr.real_opcode(), nextop) {
-                    (Some(Opcode::LoadFast), _) => {
-                        if matches!(nextop, Some(Opcode::LoadFast)) {
-                            let (inst1, rest) = block.instructions[i..].split_at_mut(1);
-                            InstructionInfo::make_super_instruction(
-                                &mut inst1[0],
-                                &mut rest[0],
-                                Opcode::LoadFastLoadFast.into(),
-                            );
-                        }
-                    }
-
-                    (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => {
-                        let (inst1, rest) = block.instructions[i..].split_at_mut(1);
-                        InstructionInfo::make_super_instruction(
-                            &mut inst1[0],
-                            &mut rest[0],
-                            Opcode::StoreFastLoadFast.into(),
-                        );
-                    }
-
-                    (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => {
-                        let (inst1, rest) = block.instructions[i..].split_at_mut(1);
-                        InstructionInfo::make_super_instruction(
-                            &mut inst1[0],
-                            &mut rest[0],
-                            Opcode::StoreFastStoreFast.into(),
-                        );
-                    }
-
-                    (_, _) => {}
-                }
+                let super_op = match (block.instructions[i].instr.real_opcode(), nextop) {
+                    (Some(Opcode::LoadFast), Some(Opcode::LoadFast)) => {
+                        Some(Opcode::LoadFastLoadFast)
+                    }
+                    (Some(Opcode::StoreFast), Some(Opcode::LoadFast)) => {
+                        Some(Opcode::StoreFastLoadFast)
+                    }
+                    (Some(Opcode::StoreFast), Some(Opcode::StoreFast)) => {
+                        Some(Opcode::StoreFastStoreFast)
+                    }
+                    (_, _) => None,
+                };
+                if let Some(super_op) = super_op {
+                    let (inst1, rest) = block.instructions[i..].split_at_mut(1);
+                    InstructionInfo::make_super_instruction(
+                        &mut inst1[0],
+                        &mut rest[0],
+                        super_op.into(),
+                    );
+                }
🤖 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 2682 - 2713, Refactor the match in
ir.rs that builds super-instructions so the repeated split_at_mut(1) and
InstructionInfo::make_super_instruction call happens only once. In the match
around block.instructions[i].instr.real_opcode(), first determine the super_op
for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast,
StoreFast/StoreFast), then perform the shared mutation once using that opcode;
the current nested if in the LoadFast arm should be treated as a direct
(Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 2682-2713: Refactor the match in ir.rs that builds
super-instructions so the repeated split_at_mut(1) and
InstructionInfo::make_super_instruction call happens only once. In the match
around block.instructions[i].instr.real_opcode(), first determine the super_op
for the three supported pairs (LoadFast/LoadFast, StoreFast/LoadFast,
StoreFast/StoreFast), then perform the shared mutation once using that opcode;
the current nested if in the LoadFast arm should be treated as a direct
(Some(Opcode::LoadFast), Some(Opcode::LoadFast)) case.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: eb7380ae-bd2f-41ca-9d7c-3c0fce2b6daa

📥 Commits

Reviewing files that changed from the base of the PR and between f196dc4 and 5456caf.

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

@youknowone
youknowone merged commit 045a6d5 into RustPython:main Jul 4, 2026
49 of 50 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