Skip to content

Make use of Unary opcodes #6647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 10 commits into from
Jan 5, 2026
Merged

Make use of Unary opcodes #6647

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
eece3bf
Make use of `Unary` opcodes
ShaharNaveh Jan 4, 2026
fd3ef1d
Fix jit
ShaharNaveh Jan 5, 2026
c54b881
Merge remote-tracking branch 'upstream/main' into bytecode-split-unary
ShaharNaveh Jan 5, 2026
15d9e0a
Add `Reserved140` opcode
ShaharNaveh Jan 5, 2026
5641bc9
re-add support for UnaryPositive in JIT
ShaharNaveh Jan 5, 2026
cf8cf21
resolve arg
ShaharNaveh Jan 5, 2026
f12d2f3
Use correct instruction
ShaharNaveh Jan 5, 2026
50763ce
JIT support for `ToBool` instruction
ShaharNaveh Jan 5, 2026
0696124
Fix unarynot
ShaharNaveh Jan 5, 2026
f86f0d5
Merge remote-tracking branch 'upstream/main' into bytecode-split-unary
ShaharNaveh Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions crates/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5656,13 +5656,20 @@ impl Compiler {
self.compile_expression(operand)?;

// Perform operation:
let op = match op {
UnaryOp::UAdd => bytecode::UnaryOperator::Plus,
UnaryOp::USub => bytecode::UnaryOperator::Minus,
UnaryOp::Not => bytecode::UnaryOperator::Not,
UnaryOp::Invert => bytecode::UnaryOperator::Invert,
match op {
UnaryOp::UAdd => emit!(
self,
Instruction::CallIntrinsic1 {
func: bytecode::IntrinsicFunction1::UnaryPositive
}
),
UnaryOp::USub => emit!(self, Instruction::UnaryNegative),
UnaryOp::Not => {
emit!(self, Instruction::ToBool);
emit!(self, Instruction::UnaryNot);
}
UnaryOp::Invert => emit!(self, Instruction::UnaryInvert),
};
emit!(self, Instruction::UnaryOperation { op });
}
Expr::Attribute(ExprAttribute { value, attr, .. }) => {
self.compile_expression(value)?;
Expand Down
37 changes: 9 additions & 28 deletions crates/compiler-core/src/bytecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ op_arg_enum!(
ImportStar = 2,
// StopIterationError = 3,
// AsyncGenWrap = 4,
// UnaryPositive = 5,
UnaryPositive = 5,
/// Convert list to tuple
ListToTuple = 6,
/// Type parameter related
Expand Down Expand Up @@ -758,11 +758,11 @@ pub enum Instruction {
StoreSubscript,
// 40: TO_BOOL
ToBool,
// 41: UNARY_INVERT - placeholder (RustPython uses UnaryOperation)
// 41: UNARY_INVERT
UnaryInvert,
// 42: UNARY_NEGATIVE - placeholder
// 42: UNARY_NEGATIVE
UnaryNegative,
// 43: UNARY_NOT - placeholder
// 43: UNARY_NOT
UnaryNot,
// ==================== With-argument instructions (opcode >= 44) ====================
// 44: WITH_EXCEPT_START
Expand Down Expand Up @@ -1091,10 +1091,6 @@ pub enum Instruction {
SetExcInfo,
// 139: SUBSCRIPT
Subscript,
// 140: UNARY_OP (combines UNARY_*)
UnaryOperation {
op: Arg<UnaryOperator>,
},
// 141-148: Reserved (padding to keep RESUME at 149)
Copy link
Copy Markdown
Member

@youknowone youknowone Jan 4, 2026

Choose a reason for hiding this comment

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

Let’s keep this line by replacing it with Reserved140 instead of removing it.

Reserved141,
Reserved142,
Expand Down Expand Up @@ -1538,18 +1534,6 @@ impl fmt::Display for BinaryOperator {
}
}

op_arg_enum!(
/// The possible unary operators
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(u8)]
pub enum UnaryOperator {
Not = 0,
Invert = 1,
Minus = 2,
Plus = 3,
}
);

op_arg_enum!(
/// Whether or not to invert the operation.
#[repr(u8)]
Expand Down Expand Up @@ -1910,13 +1894,10 @@ impl Instruction {
/// # Examples
///
/// ```
/// use rustpython_compiler_core::bytecode::{Arg, Instruction, Label, UnaryOperator};
/// use rustpython_compiler_core::bytecode::{Arg, Instruction, Label};
/// let (target, jump_arg) = Arg::new(Label(0xF));
/// let jump_instruction = Instruction::Jump { target };
/// let (op, invert_arg) = Arg::new(UnaryOperator::Invert);
/// let invert_instruction = Instruction::UnaryOperation { op };
/// assert_eq!(jump_instruction.stack_effect(jump_arg, true), 0);
/// assert_eq!(invert_instruction.stack_effect(invert_arg, false), 0);
/// ```
///
pub fn stack_effect(&self, arg: OpArg, jump: bool) -> i32 {
Expand All @@ -1925,8 +1906,7 @@ impl Instruction {
Cache | Reserved3 | Reserved17 | Reserved141 | Reserved142 | Reserved143
| Reserved144 | Reserved145 | Reserved146 | Reserved147 | Reserved148 => 0,
BinarySlice | EndFor | ExitInitCheck | GetYieldFromIter | InterpreterExit
| LoadAssertionError | LoadLocals | PushNull | ReturnGenerator | StoreSlice
| UnaryInvert | UnaryNegative | UnaryNot => 0,
| LoadAssertionError | LoadLocals | PushNull | ReturnGenerator | StoreSlice => 0,
BuildConstKeyMap { .. }
| CopyFreeVars { .. }
| DictMerge { .. }
Expand Down Expand Up @@ -1959,7 +1939,6 @@ impl Instruction {
StoreAttr { .. } => -2,
DeleteAttr { .. } => -1,
LoadConst { .. } => 1,
UnaryOperation { .. } => 0,
BinaryOp { .. } | CompareOperation { .. } => -1,
BinarySubscript => -1,
CopyItem { .. } => 1,
Expand Down Expand Up @@ -2086,6 +2065,9 @@ impl Instruction {
MatchKeys => 1, // Pop 2 (subject, keys), push 3 (subject, keys_or_none, values_or_none)
MatchClass(_) => -2,
ExtendedArg => 0,
UnaryInvert => 0,
UnaryNegative => 0,
UnaryNot => 0,
}
}

Expand Down Expand Up @@ -2302,7 +2284,6 @@ impl Instruction {
Subscript => w!(SUBSCRIPT),
Swap { index } => w!(SWAP, index),
ToBool => w!(TO_BOOL),
UnaryOperation { op } => w!(UNARY_OP, ?op),
UnpackEx { args } => w!(UNPACK_EX, args),
UnpackSequence { size } => w!(UNPACK_SEQUENCE, size),
WithExceptStart => w!(WITH_EXCEPT_START),
Expand Down
28 changes: 11 additions & 17 deletions crates/jit/src/instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use cranelift::prelude::*;
use num_traits::cast::ToPrimitive;
use rustpython_compiler_core::bytecode::{
self, BinaryOperator, BorrowedConstant, CodeObject, ComparisonOperator, Instruction, Label,
OpArg, OpArgState, UnaryOperator,
OpArg, OpArgState,
};
use std::collections::HashMap;

Expand Down Expand Up @@ -620,28 +620,22 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
self.stack.swap(i, j);
Ok(())
}
Instruction::UnaryOperation { op, .. } => {
let op = op.get(arg);
Instruction::UnaryNot => {
let a = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
match (op, a) {
(UnaryOperator::Minus, JitValue::Int(val)) => {
// Compile minus as 0 - 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(())
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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),
}
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jan 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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:

  • ~5 should return -6, but this returns -5
  • ~0 should return -1, but this returns 0
  • ~(-3) should return 2, but this returns 3
🔎 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.

Suggested change
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.

Comment on lines +653 to 664
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jan 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 -20

Repository: 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.

Expand Down
44 changes: 19 additions & 25 deletions crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1652,7 +1652,6 @@ impl ExecutingFrame<'_> {
self.push_value(vm.ctx.new_bool(bool_val).into());
Ok(None)
}
bytecode::Instruction::UnaryOperation { op } => self.execute_unary_op(vm, op.get(arg)),
bytecode::Instruction::UnpackEx { args } => {
let args = args.get(arg);
self.execute_unpack_ex(vm, args.before, args.after)
Expand Down Expand Up @@ -1762,7 +1761,24 @@ impl ExecutingFrame<'_> {
.map_err(|_| vm.new_type_error("exception expected".to_owned()))?;
Err(exc)
}

bytecode::Instruction::UnaryInvert => {
let a = self.pop_value();
let value = vm._invert(&a)?;
self.push_value(value);
Ok(None)
}
bytecode::Instruction::UnaryNegative => {
let a = self.pop_value();
let value = vm._neg(&a)?;
self.push_value(value);
Ok(None)
}
bytecode::Instruction::UnaryNot => {
let obj = self.pop_value();
let value = obj.try_to_bool(vm)?;
self.push_value(vm.ctx.new_bool(!value).into());
Ok(None)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Placeholder/dummy instructions - these should never be executed
bytecode::Instruction::Cache
| bytecode::Instruction::Reserved3
Expand All @@ -1776,9 +1792,6 @@ impl ExecutingFrame<'_> {
| bytecode::Instruction::PushNull
| bytecode::Instruction::ReturnGenerator
| bytecode::Instruction::StoreSlice
| bytecode::Instruction::UnaryInvert
| bytecode::Instruction::UnaryNegative
| bytecode::Instruction::UnaryNot
| bytecode::Instruction::BuildConstKeyMap { .. }
| bytecode::Instruction::CopyFreeVars { .. }
| bytecode::Instruction::DictMerge { .. }
Expand Down Expand Up @@ -2440,26 +2453,6 @@ impl ExecutingFrame<'_> {
Ok(None)
}

#[cfg_attr(feature = "flame-it", flame("Frame"))]
fn execute_unary_op(
&mut self,
vm: &VirtualMachine,
op: bytecode::UnaryOperator,
) -> FrameResult {
let a = self.pop_value();
let value = match op {
bytecode::UnaryOperator::Minus => vm._neg(&a)?,
bytecode::UnaryOperator::Plus => vm._pos(&a)?,
bytecode::UnaryOperator::Invert => vm._invert(&a)?,
bytecode::UnaryOperator::Not => {
let value = a.try_to_bool(vm)?;
vm.ctx.new_bool(!value).into()
}
};
self.push_value(value);
Ok(None)
}

#[cold]
fn setup_annotations(&mut self, vm: &VirtualMachine) -> FrameResult {
let __annotations__ = identifier!(vm, __annotations__);
Expand Down Expand Up @@ -2634,6 +2627,7 @@ impl ExecutingFrame<'_> {
self.import_star(vm)?;
Ok(vm.ctx.none())
}
bytecode::IntrinsicFunction1::UnaryPositive => vm._pos(&arg),
bytecode::IntrinsicFunction1::SubscriptGeneric => {
// Used for PEP 695: Generic[*type_params]
crate::builtins::genericalias::subscript_generic(arg, vm)
Expand Down
Loading