Skip to content

Newtype var_nums oparg#7431

Open
ShaharNaveh wants to merge 2 commits intoRustPython:mainfrom
ShaharNaveh:bytecode-varnums-oparg
Open

Newtype var_nums oparg#7431
ShaharNaveh wants to merge 2 commits intoRustPython:mainfrom
ShaharNaveh:bytecode-varnums-oparg

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 14, 2026

Summary by CodeRabbit

  • Refactor
    • Refactored variable index extraction across bytecode instructions, replacing manual bit-level operations with a unified accessor method for improved consistency.
    • Updated instruction field handling to use a consolidated variable numbering approach.
    • Streamlined index encoding and retrieval in the runtime and JIT compiler for better maintainability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 14, 2026

📝 Walkthrough

Walkthrough

This PR refactors the bytecode instruction type system by introducing a new VarNums type for encoding multiple variable operands and replacing manual bit-shifting logic with a dedicated indexes() method. The changes propagate through instruction variants, display logic, JIT, and VM frame handling to use the new type-safe accessor pattern.

Changes

Cohort / File(s) Summary
Oparg Type System Refactoring
crates/compiler-core/src/bytecode/oparg.rs
Type names reorganized: LoadAttr → VarNums, LoadSuperAttr → LoadAttr, Label → LoadSuperAttr, StoreFastLoadFast → Label. New VarNums impl provides idx_1(), idx_2(), and indexes() accessors replacing separate store_idx/load_idx logic.
Instruction Variant Updates
crates/compiler-core/src/bytecode/instruction.rs
Several bytecode instructions updated to use Arg<oparg::VarNums> for var_nums fields instead of Arg<u32> or custom types. Display logic for LOAD_FAST_LOAD_FAST, LOAD_FAST_BORROW_LOAD_FAST_BORROW, STORE_FAST_LOAD_FAST, and STORE_FAST_STORE_FAST now calls oparg.indexes() instead of manual bit-shifting. Removed StoreFastLoadFast from public imports.
Index Extraction Simplification
crates/jit/src/instructions.rs, crates/vm/src/frame.rs
Replaced manual index extraction via bit-shift operations (idx = oparg >> 4; idx = oparg & 0xF) with structured oparg.indexes() calls. Frame handling updated to use var_nums.get(arg).indexes() for loading and storing variable pairs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 Hop, skip, and a bound—old bit-shifts retire,
New indexes() dance where opcodes conspire!
VarNums embrace the variables with grace,
No more shifting masks all over the place!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Newtype var_nums oparg' accurately describes the main change: introducing a new VarNums type that replaces several oparg-like types and consolidates their accessor logic.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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
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 (2)
crates/compiler-core/src/bytecode/oparg.rs (1)

757-771: Use named constants for nibble packing to remove magic numbers.

Logic is correct, but replacing 4 and 15 with constants makes the encoding contract clearer and safer to maintain.

Suggested diff
 impl VarNums {
+    const LOW_NIBBLE_MASK: u32 = 0x0F;
+    const HIGH_NIBBLE_SHIFT: u32 = 4;
+
     #[must_use]
     pub const fn idx_1(self) -> VarNum {
-        VarNum::new(self.0 >> 4)
+        VarNum::new(self.0 >> Self::HIGH_NIBBLE_SHIFT)
     }

     #[must_use]
     pub const fn idx_2(self) -> VarNum {
-        VarNum::new(self.0 & 15)
+        VarNum::new(self.0 & Self::LOW_NIBBLE_MASK)
     }

     #[must_use]
     pub const fn indexes(self) -> (VarNum, VarNum) {
         (self.idx_1(), self.idx_2())
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 757 - 771, Replace
the magic numbers in VarNums's nibble packing/unpacking by adding descriptive
consts (e.g., NIBBLE_BITS = 4 and NIBBLE_MASK = (1 << NIBBLE_BITS) - 1) and use
them in the methods idx_1, idx_2, and indexes; update VarNums::idx_1 to shift by
NIBBLE_BITS and VarNums::idx_2 to mask with NIBBLE_MASK while keeping them as
const fn and returning VarNum::new(...) so the encoding contract is explicit and
easier to maintain.
crates/compiler-core/src/bytecode/instruction.rs (1)

1348-1353: Inconsistent display format for StoreFastLoadFast.

StoreFastLoadFast outputs raw index numbers (store_idx, load_idx) instead of variable names, while similar instructions like LoadFastLoadFast, LoadFastBorrowLoadFastBorrow, and StoreFastStoreFast all use varname() to display human-readable variable names.

Consider using varname() for consistency:

♻️ Suggested fix for consistency
 Self::StoreFastLoadFast { var_nums } => {
     let oparg = var_nums.get(arg);
     let (store_idx, load_idx) = oparg.indexes();
-    write!(f, "STORE_FAST_LOAD_FAST")?;
-    write!(f, " ({}, {})", store_idx, load_idx)
+    write!(
+        f,
+        "{:pad$}({}, {})",
+        "STORE_FAST_LOAD_FAST",
+        varname(store_idx),
+        varname(load_idx)
+    )
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1348 - 1353,
The Display implementation for the enum variant Self::StoreFastLoadFast {
var_nums } prints raw indexes (store_idx, load_idx) rather than human-readable
names; change the second write! call in that match arm to use varname() on the
indexes (the same approach used by LoadFastLoadFast,
LoadFastBorrowLoadFastBorrow, and StoreFastStoreFast) so it prints the variable
names (e.g., var_nums.varname(store_idx), var_nums.varname(load_idx)) instead of
numeric indices while preserving the "STORE_FAST_LOAD_FAST" label.
🤖 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/compiler-core/src/bytecode/instruction.rs`:
- Around line 1348-1353: The Display implementation for the enum variant
Self::StoreFastLoadFast { var_nums } prints raw indexes (store_idx, load_idx)
rather than human-readable names; change the second write! call in that match
arm to use varname() on the indexes (the same approach used by LoadFastLoadFast,
LoadFastBorrowLoadFastBorrow, and StoreFastStoreFast) so it prints the variable
names (e.g., var_nums.varname(store_idx), var_nums.varname(load_idx)) instead of
numeric indices while preserving the "STORE_FAST_LOAD_FAST" label.

In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 757-771: Replace the magic numbers in VarNums's nibble
packing/unpacking by adding descriptive consts (e.g., NIBBLE_BITS = 4 and
NIBBLE_MASK = (1 << NIBBLE_BITS) - 1) and use them in the methods idx_1, idx_2,
and indexes; update VarNums::idx_1 to shift by NIBBLE_BITS and VarNums::idx_2 to
mask with NIBBLE_MASK while keeping them as const fn and returning
VarNum::new(...) so the encoding contract is explicit and easier to maintain.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d09d8e27-02fc-4171-8d40-a1b168537389

📥 Commits

Reviewing files that changed from the base of the PR and between 40fc7c2 and 41b2914.

📒 Files selected for processing (4)
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/jit/src/instructions.rs
  • crates/vm/src/frame.rs

#[derive(Clone, Copy)]
#[repr(transparent)]
pub struct LoadAttr(u32)
pub struct VarNums(u32)
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a pair of u16, right? can this be written using union?

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Mar 14, 2026

Choose a reason for hiding this comment

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

Maybe this can be written as a union, I'm not familiar enough with them to properly implement it. Also, I ain't sure how much this will help performance/memory utilization, in comparison with the effort to refactor things (if needed?)

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