Unify BINARY_OP bytecodes #6317

ShaharNaveh · 2025-12-01T21:50:36Z

Summary by CodeRabbit

Refactor
- Unified binary operation handling into a single instruction form and consolidated execution/runtime paths.
- Introduced explicit in‑place operator variants and standardized their runtime mapping.
User-visible
- Operator symbols and textual formatting standardized for clarity.
- Documentation and examples updated to reflect the unified representation and clarified true‑division vs remainder semantics.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-01T21:50:48Z

Walkthrough

Replaces separate BinaryOperation/BinaryOperationInplace instruction variants with a single Instruction::BinaryOp { op }. BinaryOperator now includes explicit in-place variants and an as_inplace() method plus Display. Codegen emits BinaryOp; VM and JIT route inplace and non-inplace operators via unified handling.

Changes

Cohort / File(s)	Summary
Bytecode Definition `crates/compiler-core/src/bytecode.rs`	Replaced `BinaryOperation` and `BinaryOperationInplace` with `BinaryOp { op }`. Expanded `BinaryOperator` with explicit in-place variants, added `pub const fn as_inplace(self)` and `impl fmt::Display`. Updated `stack_effect`, `fmt_dis`, and docs/examples to use `BinaryOp`.
Codegen Emission `crates/codegen/src/compile.rs`	Refactored local operator mapping to the new `BinaryOperator` enum; in-place uses `op.as_inplace()` and emission unified to `Instruction::BinaryOp { op }`.
VM Execution `crates/vm/src/frame.rs`	Consolidated binary op handling to a single `BinaryOp { op }` path. Added match arms for new in-place variants mapping to `_i*` VM methods; adjusted Divide→`TrueDivide` and Modulo→`Remainder`. Removed `execute_bin_op_inplace`.
JIT Handling `crates/jit/src/instructions.rs`	Replaced dispatch for old BinaryOperation variants with `BinaryOp { op }`. Updated integer/float division and remainder handling to use `TrueDivide`/`Remainder` (including Inplace variants); preserved existing overflow/trap behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Codegen
  participant Bytecode
  participant VM
  participant JIT

  Note over Codegen,Bytecode: Compile-time
  Codegen->>Bytecode: emit Instruction::BinaryOp { op } (use op.as_inplace() when inplace)
  Note right of Bytecode: BinaryOp holds BinaryOperator (incl. inplace variants)

  Note over VM,JIT: Runtime
  VM->>Bytecode: fetch Instruction::BinaryOp { op }
  VM->>VM: match op
  alt inplace variant
    VM->>VM: invoke corresponding _i* method (e.g., _iadd, _isub, _imul)
  else non-inplace
    VM->>VM: invoke non-inplace method (e.g., _add, _sub, _truediv)
  end
  VM->>VM: push result on stack

  JIT->>Bytecode: receive/translate BinaryOp { op }
  JIT->>JIT: lower op (TrueDivide/Remainder adjustments) and emit native sequence

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Verify BinaryOperator::as_inplace() covers all operators and maps correctly.
Check VM match arms call the correct _i* or non-inplace methods and preserve edge-case semantics (division-by-zero, mixed-type promotions).
Ensure codegen emits all operator cases (including inplace) and tests/emit paths updated.
Review JIT integer/float division and remainder handling for parity with VM and proper traps.

Suggested reviewers

youknowone
coolreader18

Poem

🐰 I hop through bytes and stitch the ops as one,

In-place and plain now share a single run.
One instruction hums, light and spry,
VM and JIT leap, then stack results fly.
Carrots for tests — adventure done! 🥕

Pre-merge checks and finishing touches

✅ 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 'Unify BINARY_OP bytecodes' directly and clearly summarizes the main change: consolidating multiple binary operation instruction variants into a single unified variant.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c9e00b and c7c5e67.

📒 Files selected for processing (1)

crates/jit/src/instructions.rs (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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/jit/src/instructions.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Check Rust code with clippy
GitHub Check: Ensure compilation on various targets
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run tests under miri
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)

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

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e0a86d and 9cc108d.

📒 Files selected for processing (3)

crates/codegen/src/compile.rs (2 hunks)
crates/compiler-core/src/bytecode.rs (4 hunks)
crates/vm/src/frame.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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
crates/compiler-core/src/bytecode.rs
crates/vm/src/frame.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check Rust code with clippy
GitHub Check: Run tests under miri
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (9)

crates/codegen/src/compile.rs (2)

3304-3307: LGTM on the binary operation refactor in pattern matching.

The subtraction operation for computing negative sequence indices is correctly updated to use the unified Instruction::BinaryOp with BinaryOperator::Subtract.

4341-4359: Verify BinaryOperator::as_inplace() method availability.

The operator mapping is exhaustive and correctly structured, but the implementation depends on the as_inplace() method existing on the BinaryOperator type. This method must be verified to exist and properly convert each binary operator to its in-place variant. Confirm the method is implemented in the bytecode module before merging.

crates/vm/src/frame.rs (2)

871-871: LGTM! Instruction consolidation aligns with PR objective.

The unification of binary operation instruction variants into a single BinaryOp { op } is correctly implemented.

2117-2153: LGTM! Binary operation consolidation is well-implemented.

The unified execute_bin_op method correctly handles both regular and inplace operations. The operator naming (TrueDivide, Remainder) aligns with Python 3 semantics, and all inplace variants follow a consistent pattern.

crates/compiler-core/src/bytecode.rs (5)

597-599: LGTM! Instruction enum updated correctly.

The consolidation into a single BinaryOp { op } variant aligns with the PR objective and matches the usage in frame.rs.

1106-1160: LGTM! Comprehensive BinaryOperator enum definition.

The expanded enum with explicit values and symbol annotations provides a complete set of binary operations, ensuring stable bytecode representation.

1192-1224: LGTM! Complete and correct Display implementation.

All binary operators are mapped to their correct symbol representations, covering both regular and inplace variants.

1620-1620: LGTM! Correct stack effect for unified BinaryOp.

The stack effect of -1 is correct for all binary operations (pops 2 operands, pushes 1 result).

1825-1825: LGTM! Disassembly formatting updated correctly.

The fmt_dis implementation correctly displays the operator symbol using the Display trait, producing clear disassembly output like "BINARY_OP +".

crates/compiler-core/src/bytecode.rs

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc1d6bb and f3c0dc3.

📒 Files selected for processing (1)

crates/compiler-core/src/bytecode.rs (4 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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/compiler-core/src/bytecode.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run tests under miri
GitHub Check: Check the WASM package and demo
GitHub Check: Check Rust code with clippy
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (7)

crates/compiler-core/src/bytecode.rs (7)

597-599: LGTM! Instruction unification aligns with CPython.

The unification of BinaryOperation and BinaryOperationInplace into a single BinaryOp instruction correctly implements the CPython approach referenced in the PR objectives.

1094-1105: LGTM! Documentation example is now correct.

The documentation example correctly imports the required types and demonstrates proper usage of the BinaryOp instruction. This addresses the previous review feedback.

1107-1161: LGTM! Complete operator set with clear documentation.

All 13 basic binary operators and their 13 in-place variants are properly defined with clear symbol documentation. The enum values (0-25) are sequential and fit well within the u8 representation.

1174-1191: LGTM! Complete and correct mapping in as_inplace().

All 13 non-inplace operators correctly map to their in-place equivalents, and the wildcard arm properly handles operators that are already in-place. This addresses the previous review feedback about the missing And => InplaceAnd mapping.

1194-1226: LGTM! Comprehensive Display implementation.

All 26 operators have their correct string representations, making disassembly output clear and readable. The implementation correctly distinguishes between non-inplace (e.g., "+") and in-place (e.g., "+=") operators.

1622-1622: LGTM! Stack effect correctly unified.

The unified BinaryOp instruction correctly returns a stack effect of -1, which is appropriate for all binary operations (pop 2 operands, push 1 result).

1827-1827: LGTM! Improved disassembly formatting.

The formatting now uses the Display trait implementation, producing clearer output with operator symbols (e.g., "BINARY_OP +" or "BINARY_OP +=") instead of enum variant names.

crates/compiler-core/src/bytecode.rs

youknowone

👍 Great, thank you!

Unify BINARY_OP bytecodes

9cc108d

coderabbitai bot reviewed Dec 1, 2025

View reviewed changes

crates/compiler-core/src/bytecode.rs Show resolved Hide resolved

crates/compiler-core/src/bytecode.rs Show resolved Hide resolved

ShaharNaveh added 3 commits December 2, 2025 00:03

Add missing op to as_inplace

bc1d6bb

Fix doc example

f3c0dc3

Fix jit

52d3b45

coderabbitai bot reviewed Dec 1, 2025

View reviewed changes

crates/compiler-core/src/bytecode.rs Show resolved Hide resolved

ShaharNaveh added 4 commits December 2, 2025 00:11

Fix doc

c9886c3

Use correct opname

63d0724

Fix dis fmt

0c9e00b

Inplace ops support in JIT

c7c5e67

youknowone approved these changes Dec 1, 2025

View reviewed changes

youknowone merged commit 9130dd8 into RustPython:main Dec 2, 2025
13 checks passed

coderabbitai bot mentioned this pull request Dec 2, 2025

Sort Instruction enum & related match arms #6322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unify BINARY_OP bytecodes #6317

Unify BINARY_OP bytecodes #6317

Uh oh!

ShaharNaveh commented Dec 1, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 1, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Unify BINARY_OP bytecodes #6317

Unify BINARY_OP bytecodes #6317

Uh oh!

Conversation

ShaharNaveh commented Dec 1, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Dec 1, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 1, 2025 •

edited

Loading