Make use of Unary opcodes#6647
Conversation
📝 WalkthroughWalkthroughReplaces the previous generic unary dispatch with explicit per-variant bytecode and intrinsic instructions across codegen, bytecode, VM, and JIT: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CG as Codegen
participant BC as Bytecode
participant VM as VM
participant JIT as JIT
rect rgb(235,245,255)
Note over CG,BC: Codegen emits explicit unary opcodes/intrinsics
end
CG->>BC: Emit CallIntrinsic1(UnaryPositive) / UnaryNegative / UnaryInvert / ToBool + UnaryNot
BC->>VM: Bytecode stream (explicit unary opcodes)
VM->>JIT: Request compiled handler or fallback to interpreter
alt UnaryPositive (UAdd)
VM->>VM: CallIntrinsic1 handler → vm._pos(arg)
VM->>VM: Push result
else UnaryNegative
VM->>VM: Pop operand → apply negation → Push result
else UnaryInvert
VM->>VM: Pop operand → apply bitwise invert (int) or NotSupported → Push result
else Not (ToBool + UnaryNot)
VM->>VM: Execute ToBool → UnaryNot (bool flip) → Push result
end
rect rgb(245,245,245)
Note over VM,JIT: Generic UnaryOperation dispatch removed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ 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)
🔇 Additional comments (3)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/codegen/src/compile.rs (1)
5659-5672: Unary op emission matches new opcode/intrinsic splitThe new mapping looks consistent with the refactor:
UAddgoes throughCallIntrinsic1::UnaryPositive, whileUSub,Not, andInverttarget the dedicatedUnaryNegative/UnaryNot/UnaryInvertinstructions. Stack usage is correct (operand compiled first, each op consuming/producing a single value), andnotis now clearly desugared toToBool+UnaryNot, matching truthiness semantics and the separatecompile_jump_ifhandling for control-flow cases.I don’t see any changes needed here; just make sure there are bytecode/VM tests covering
+x,-x,~x, andnot xto catch regressions across the new opcodes/intrinsic.crates/vm/src/frame.rs (1)
2630-2630: UnaryPositive intrinsic implementation looks correct.The call to
vm._pos(&arg)properly handles the unary plus operation.However, note the architectural inconsistency:
UnaryInvert,UnaryNegative, andUnaryNotare direct instructions, whileUnaryPositiveis handled via intrinsic. This may be intentional (e.g., if unary plus is less common or has special semantics), but it creates a mixed approach to unary operations.💡 Optional: Consider architectural consistency
For consistency, consider whether all unary operations should follow the same pattern (either all direct instructions or all intrinsics). The current mixed approach is:
- Direct instructions:
UnaryInvert,UnaryNegative,UnaryNot- Intrinsic:
UnaryPositiveIf there's a specific reason for this split (e.g., unary plus is rarely used), it might be worth documenting in a code comment.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/vm/src/frame.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint 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.rscrates/vm/src/frame.rscrates/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). (8)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
crates/vm/src/frame.rs (1)
1764-1781: LGTM! Clean implementation of explicit unary operations.The three unary instruction implementations are correct and straightforward:
- Each properly pops the operand, applies the operation, and pushes the result
- Error handling is appropriate
UnaryNotcorrectly callstry_to_boolbefore negating, which matches Python semanticscrates/compiler-core/src/bytecode.rs (3)
643-643: LGTM! UnaryPositive intrinsic variant added correctly.The
UnaryPositive = 5variant is properly added to theIntrinsicFunction1enum with the correct sequential value.
761-766: LGTM! Explicit unary instruction variants correctly added.The three unary instructions (
UnaryInvert,UnaryNegative,UnaryNot) are:
- Correctly positioned as no-argument instructions (opcodes 41-43, below the 44 threshold)
- Properly documented with their opcode numbers
- Consistent with the CPython 3.13 opcode structure
2068-2070: LGTM! Stack effects and display formatting are correct.
- Stack effects (lines 2068-2070): All three unary operations correctly show a stack effect of 0 (pop 1 operand, push 1 result)
- Display formatting (lines 2155-2157): Proper use of the
w!macro for instruction display, consistent with other instructions- The unary operations are correctly excluded from the placeholder/dummy instruction list
Also applies to: 2155-2157
crates/compiler-core/src/bytecode.rs
Outdated
| UnaryOperation { | ||
| op: Arg<UnaryOperator>, | ||
| }, | ||
| // 141-148: Reserved (padding to keep RESUME at 149) |
There was a problem hiding this comment.
Let’s keep this line by replacing it with Reserved140 instead of removing it.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @crates/jit/src/instructions.rs:
- Around line 630-641: The UnaryInvert arm incorrectly implements bitwise NOT by
computing 0 - val (negation); change the logic in Instruction::UnaryInvert
handling for JitValue::Int so it produces bitwise complement: either emit a bnot
on the integer value via the builder (if Cranelift supports a bnot op) or
compute -val - 1 by calling compile_sub twice (first compute neg =
compile_sub(zero, val) then compute out = compile_sub(neg, one)) where one is an
iconst types::I64 value of 1; push JitValue::Int(out) and keep the NotSupported
error path unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/jit/src/instructions.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint 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
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: RustPython is a Python 3 interpreter written in Rust implementing Python 3.13.0+ compatibility with complete Python environment in Rust (not CPython bindings), clean implementation without compatibility hacks, and cross-platform support including WebAssembly compilation
Applied to 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: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (2)
crates/jit/src/instructions.rs (2)
8-8: LGTM: Import updates align with the refactor.The removal of
UnaryOperatorfrom imports is correct and consistent with the broader refactoring that replaces the composite unary operation pattern with explicit per-variant instructions.
623-629: LGTM: Correct implementation of logical NOT.The implementation correctly converts the operand to a boolean value and negates it using XOR with 1. This properly implements Python's
notoperator semantics.
crates/jit/src/instructions.rs
Outdated
| Instruction::UnaryInvert => { | ||
| match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { | ||
| JitValue::Int(val) => { | ||
| // Compile minus as 0 - val. | ||
| let zero = self.builder.ins().iconst(types::I64, 0); | ||
| let out = self.compile_sub(zero, val); | ||
| self.stack.push(JitValue::Int(out)); | ||
| Ok(()) | ||
| } | ||
| (UnaryOperator::Plus, JitValue::Int(val)) => { | ||
| // Nothing to do | ||
| self.stack.push(JitValue::Int(val)); | ||
| Ok(()) | ||
| } | ||
| (UnaryOperator::Not, a) => { | ||
| let boolean = self.boolean_val(a)?; | ||
| let not_boolean = self.builder.ins().bxor_imm(boolean, 1); | ||
| self.stack.push(JitValue::Bool(not_boolean)); | ||
| Ok(()) | ||
| } | ||
| _ => Err(JitCompileError::NotSupported), | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect implementation of bitwise invert operator.
The UnaryInvert instruction implements Python's ~ operator (bitwise NOT), which should compute -x - 1 for integers, not -x. The current implementation computes 0 - val, which is negation, producing incorrect results.
Examples of the bug:
~5should return-6, but this returns-5~0should return-1, but this returns0~(-3)should return2, but this returns3
🔎 Proposed fix for bitwise invert
Instruction::UnaryInvert => {
match self.stack.pop().ok_or(JitCompileError::BadBytecode)? {
JitValue::Int(val) => {
- // Compile minus as 0 - val.
+ // Bitwise invert: ~x = -x - 1
let zero = self.builder.ins().iconst(types::I64, 0);
- let out = self.compile_sub(zero, val);
+ let neg_val = self.compile_sub(zero, val);
+ let one = self.builder.ins().iconst(types::I64, 1);
+ let out = self.compile_sub(neg_val, one);
self.stack.push(JitValue::Int(out));
Ok(())
}
_ => Err(JitCompileError::NotSupported),
}
}Alternatively, if Cranelift supports bitwise NOT directly (e.g., bnot), that would be more efficient and clearer:
Instruction::UnaryInvert => {
match self.stack.pop().ok_or(JitCompileError::BadBytecode)? {
JitValue::Int(val) => {
- // Compile minus as 0 - val.
- let zero = self.builder.ins().iconst(types::I64, 0);
- let out = self.compile_sub(zero, val);
+ // Bitwise invert using NOT instruction
+ let out = self.builder.ins().bnot(val);
self.stack.push(JitValue::Int(out));
Ok(())
}
_ => Err(JitCompileError::NotSupported),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Instruction::UnaryInvert => { | |
| match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { | |
| JitValue::Int(val) => { | |
| // Compile minus as 0 - val. | |
| let zero = self.builder.ins().iconst(types::I64, 0); | |
| let out = self.compile_sub(zero, val); | |
| self.stack.push(JitValue::Int(out)); | |
| Ok(()) | |
| } | |
| (UnaryOperator::Plus, JitValue::Int(val)) => { | |
| // Nothing to do | |
| self.stack.push(JitValue::Int(val)); | |
| Ok(()) | |
| } | |
| (UnaryOperator::Not, a) => { | |
| let boolean = self.boolean_val(a)?; | |
| let not_boolean = self.builder.ins().bxor_imm(boolean, 1); | |
| self.stack.push(JitValue::Bool(not_boolean)); | |
| Ok(()) | |
| } | |
| _ => Err(JitCompileError::NotSupported), | |
| } | |
| } | |
| Instruction::UnaryInvert => { | |
| match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { | |
| JitValue::Int(val) => { | |
| // Bitwise invert: ~x = -x - 1 | |
| let zero = self.builder.ins().iconst(types::I64, 0); | |
| let neg_val = self.compile_sub(zero, val); | |
| let one = self.builder.ins().iconst(types::I64, 1); | |
| let out = self.compile_sub(neg_val, one); | |
| self.stack.push(JitValue::Int(out)); | |
| Ok(()) | |
| } | |
| _ => Err(JitCompileError::NotSupported), | |
| } | |
| } |
🤖 Prompt for AI Agents
In @crates/jit/src/instructions.rs around lines 630-641, The UnaryInvert arm
incorrectly implements bitwise NOT by computing 0 - val (negation); change the
logic in Instruction::UnaryInvert handling for JitValue::Int so it produces
bitwise complement: either emit a bnot on the integer value via the builder (if
Cranelift supports a bnot op) or compute -val - 1 by calling compile_sub twice
(first compute neg = compile_sub(zero, val) then compute out = compile_sub(neg,
one)) where one is an iconst types::I64 value of 1; push JitValue::Int(out) and
keep the NotSupported error path unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @crates/jit/src/instructions.rs:
- Around line 644-649: The UnaryNot branch uses a JitValue enum directly in
bxor_imm; update Instruction::UnaryNot to pop the value, match it to
JitValue::Bool(var) to extract the underlying Cranelift Value (or return
JitCompileError::BadBytecode if it isn't Bool), call self.builder.ins().bxor_imm
on that extracted Value, then push JitValue::Bool(result) as before; ensure you
do not pass the enum itself to bxor_imm and preserve existing error handling
around self.stack.pop().
- Around line 650-661: The UnaryNegative handler currently only handles
JitValue::Int by compiling 0 - val; add a branch for JitValue::Float in the
match inside Instruction::UnaryNegative to call self.builder.ins().fneg on the
popped float value and push JitValue::Float(out) onto self.stack, returning
Ok(()); keep the existing Int branch (using compile_sub) and the fallback error
branch unchanged.
- Around line 477-491: The unary intrinsic handling in
Instruction::CallIntrinsic1 { func } currently only supports JitValue::Int for
IntrinsicFunction1::UnaryPositive; update the match arms so both
IntrinsicFunction1::UnaryPositive and IntrinsicFunction1::UnaryNegative handle
JitValue::Float as well (for UnaryPositive push the same JitValue::Float back,
for UnaryNegative push a JitValue::Float with the negated value), mirroring the
existing Int semantics around the
self.stack.pop().ok_or(JitCompileError::BadBytecode)? pattern and returning the
same error branches for unsupported types.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/jit/src/instructions.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint 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). (9)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (2)
crates/jit/src/instructions.rs (2)
7-8: LGTM: Import changes align with new instruction variants.The addition of
IntrinsicFunction1and restructured imports correctly support the newCallIntrinsic1instruction handler and explicit unary operation variants.
638-643: LGTM: ToBool conversion is correctly implemented.The handler properly converts any JitValue to a boolean using the existing
boolean_valhelper and pushes the result asJitValue::Bool.
| Instruction::CallIntrinsic1 { func } => { | ||
| match func.get(arg) { | ||
| IntrinsicFunction1::UnaryPositive => { | ||
| match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { | ||
| JitValue::Int(val) => { | ||
| // Nothing to do | ||
| self.stack.push(JitValue::Int(val)); | ||
| Ok(()) | ||
| } | ||
| _ => Err(JitCompileError::NotSupported), | ||
| } | ||
| } | ||
| _ => Err(JitCompileError::NotSupported), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for UnaryPositive references and Float handling patterns
rg -n 'UnaryPositive' crates/jit/src/ --type rust -C 2Repository: RustPython/RustPython
Length of output: 528
🏁 Script executed:
# Check if Float is handled for other unary operations
rg -n 'JitValue::Float' crates/jit/src/instructions.rs -C 3 | head -100Repository: RustPython/RustPython
Length of output: 5237
🏁 Script executed:
# Search for IntrinsicFunction1 definition to understand what operations exist
rg -n 'enum IntrinsicFunction1|IntrinsicFunction1::' crates/jit/src/ --type rust -B2 -A5Repository: RustPython/RustPython
Length of output: 790
🏁 Script executed:
# Check if there are any Float support patterns in the codebase
rg -n 'Float.*=>|JitValue::Float' crates/jit/src/instructions.rs --type rust -B2 -A2 | head -50Repository: RustPython/RustPython
Length of output: 2409
🏁 Script executed:
# Search for other unary operations to see the pattern
rg -n 'UnaryNegative|UnaryInvert|UnaryNot' crates/jit/src/instructions.rs --type rust -B2 -A8Repository: RustPython/RustPython
Length of output: 940
🏁 Script executed:
# Get more context for UnaryNegative to see if Float is handled
rg -n 'Instruction::UnaryNegative' crates/jit/src/instructions.rs --type rust -A20Repository: RustPython/RustPython
Length of output: 1102
Add Float support for unary operations.
The implementation correctly handles UnaryPositive for Int, but Float support is missing. This is inconsistent with binary operations throughout the file (Add, Subtract, Multiply, TrueDivide, Power all support Float). Both UnaryPositive and UnaryNegative should handle JitValue::Float with the same semantics as Int (identity for +, negate for -).
🤖 Prompt for AI Agents
In @crates/jit/src/instructions.rs around lines 477-491, The unary intrinsic
handling in Instruction::CallIntrinsic1 { func } currently only supports
JitValue::Int for IntrinsicFunction1::UnaryPositive; update the match arms so
both IntrinsicFunction1::UnaryPositive and IntrinsicFunction1::UnaryNegative
handle JitValue::Float as well (for UnaryPositive push the same JitValue::Float
back, for UnaryNegative push a JitValue::Float with the negated value),
mirroring the existing Int semantics around the
self.stack.pop().ok_or(JitCompileError::BadBytecode)? pattern and returning the
same error branches for unsupported types.
| Instruction::UnaryNegative => { | ||
| match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { | ||
| JitValue::Int(val) => { | ||
| // Compile minus as 0 - val. | ||
| let zero = self.builder.ins().iconst(types::I64, 0); | ||
| let out = self.compile_sub(zero, val); | ||
| self.stack.push(JitValue::Int(out)); | ||
| Ok(()) | ||
| } | ||
| (UnaryOperator::Plus, JitValue::Int(val)) => { | ||
| // Nothing to do | ||
| self.stack.push(JitValue::Int(val)); | ||
| Ok(()) | ||
| } | ||
| (UnaryOperator::Not, a) => { | ||
| let boolean = self.boolean_val(a)?; | ||
| let not_boolean = self.builder.ins().bxor_imm(boolean, 1); | ||
| self.stack.push(JitValue::Bool(not_boolean)); | ||
| Ok(()) | ||
| } | ||
| _ => Err(JitCompileError::NotSupported), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for fneg and Float handling in unary operations
rg -nP --type=rust -C3 'fneg|UnaryNegative.*Float|Float.*UnaryNegative' crates/jit/src/Repository: RustPython/RustPython
Length of output: 609
🏁 Script executed:
#!/bin/bash
# Check the full context around UnaryNegative and look for Float handling patterns
rg -nP --type=rust -B5 -A10 'Instruction::UnaryNegative' crates/jit/src/Repository: RustPython/RustPython
Length of output: 1435
🏁 Script executed:
#!/bin/bash
# Search for UnaryPositive to see if it has similar patterns
rg -nP --type=rust -B2 -A8 'Instruction::UnaryPositive' crates/jit/src/Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for Float handling in other unary or binary operations for reference
rg -nP --type=rust 'JitValue::Float.*=>' crates/jit/src/ | head -20Repository: RustPython/RustPython
Length of output: 556
Add Float support to UnaryNegative handler.
The implementation handles Int correctly but should also support JitValue::Float. Since the codebase uses fneg for float negation and Float is handled in other binary operations (e.g., line 529), add a case for JitValue::Float:
JitValue::Float(val) => {
let out = self.builder.ins().fneg(val);
self.stack.push(JitValue::Float(out));
Ok(())
}🤖 Prompt for AI Agents
In @crates/jit/src/instructions.rs around lines 650-661, The UnaryNegative
handler currently only handles JitValue::Int by compiling 0 - val; add a branch
for JitValue::Float in the match inside Instruction::UnaryNegative to call
self.builder.ins().fneg on the popped float value and push JitValue::Float(out)
onto self.stack, returning Ok(()); keep the existing Int branch (using
compile_sub) and the fallback error branch unchanged.
* Make use of `Unary` opcodes * Add `Reserved140` opcode * re-add support for UnaryPositive in JIT * JIT support for `ToBool` instruction
* Make use of `Unary` opcodes * Add `Reserved140` opcode * re-add support for UnaryPositive in JIT * JIT support for `ToBool` instruction
Summary by CodeRabbit
Refactor
Documentation / Tests
✏️ Tip: You can customize this high-level summary in your review settings.