Visitar URL original
Sort `Instruction` variants + Ensure each holds a singular oparg type by ShaharNaveh · Pull Request #6312 · RustPython/RustPython · GitHub
Skip to content

Sort Instruction variants + Ensure each holds a singular oparg type #6312

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

Closed
ShaharNaveh wants to merge 16 commits into from
Closed

Sort Instruction variants + Ensure each holds a singular oparg type #6312

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
423fc3d
Sort `Instruction` enum. Variants hold 1 element
ShaharNaveh Nov 29, 2025
ef456a7
Fix `fmt_dis`
ShaharNaveh Nov 29, 2025
e2a1dfa
Fix `stack_effect`
ShaharNaveh Nov 29, 2025
0eed077
Fix `label_arg`
ShaharNaveh Nov 29, 2025
e844cef
Fix codegen compile
ShaharNaveh Nov 29, 2025
7659663
Fix `execute_instruction`
ShaharNaveh Nov 29, 2025
73b5b88
Sort `execute_instruction` match arms
ShaharNaveh Nov 29, 2025
f59c99f
Clippy
ShaharNaveh Nov 29, 2025
6d52107
Fix jit `add_instruction`
ShaharNaveh Nov 29, 2025
d5abcdb
Sort match arms
ShaharNaveh Nov 29, 2025
3a01e05
Missed one
ShaharNaveh Nov 29, 2025
845cdd9
fix Jit's `process_instruction`
ShaharNaveh Nov 29, 2025
f35d028
Sort arms
ShaharNaveh Nov 29, 2025
6579723
Fix docs
ShaharNaveh Nov 29, 2025
7beb732
Merge remote-tracking branch 'upstream/main' into sort-inst
ShaharNaveh Nov 29, 2025
85b17af
Add comments
ShaharNaveh Nov 29, 2025
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
Prev Previous commit
Next Next commit
Sort match arms
  • Loading branch information
@ShaharNaveh
ShaharNaveh committed Nov 29, 2025
commit d5abcdbd5bf2eda43ad249e80128e87f11ec444a
367 changes: 183 additions & 184 deletions crates/jit/src/instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,167 +275,6 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
arg: OpArg,
) -> Result<(), JitCompileError> {
match instruction {
Instruction::ExtendedArg => Ok(()),
Instruction::PopJumpIfFalse(target) => {
let cond = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let val = self.boolean_val(cond)?;
let then_block = self.get_or_create_block(target.get(arg));
let else_block = self.builder.create_block();

self.builder
.ins()
.brif(val, else_block, &[], then_block, &[]);
self.builder.switch_to_block(else_block);

Ok(())
}
Instruction::PopJumpIfTrue(target) => {
let cond = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let val = self.boolean_val(cond)?;
let then_block = self.get_or_create_block(target.get(arg));
let else_block = self.builder.create_block();

self.builder
.ins()
.brif(val, then_block, &[], else_block, &[]);
self.builder.switch_to_block(else_block);

Ok(())
}

Instruction::Jump(target) => {
let target_block = self.get_or_create_block(target.get(arg));
self.builder.ins().jump(target_block, &[]);
Ok(())
}
Instruction::LoadFast(idx) => {
let local = self.variables[idx.get(arg) as usize]
.as_ref()
.ok_or(JitCompileError::BadBytecode)?;
self.stack.push(JitValue::from_type_and_value(
local.ty.clone(),
self.builder.use_var(local.var),
));
Ok(())
}
Instruction::StoreFast(idx) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.store_variable(idx.get(arg), val)
}
Instruction::LoadConst { idx } => {
let val = self
.prepare_const(bytecode.constants[idx.get(arg) as usize].borrow_constant())?;
self.stack.push(val);
Ok(())
}
Instruction::BuildTuple(size) => {
let elements = self.pop_multiple(size.get(arg) as usize);
self.stack.push(JitValue::Tuple(elements));
Ok(())
}
Instruction::UnpackSequence(size) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;

let elements = match val {
JitValue::Tuple(elements) => elements,
_ => return Err(JitCompileError::NotSupported),
};

if elements.len() != size.get(arg) as usize {
return Err(JitCompileError::NotSupported);
}

self.stack.extend(elements.into_iter().rev());
Ok(())
}
Instruction::ReturnValue => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.return_value(val)
}
Instruction::ReturnConst(idx) => {
let val = self
.prepare_const(bytecode.constants[idx.get(arg) as usize].borrow_constant())?;
self.return_value(val)
}
Instruction::CompareOperation(op) => {
let op = op.get(arg);
// the rhs is popped off first
let b = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let a = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;

let a_type: Option<JitType> = a.to_jit_type();
let b_type: Option<JitType> = b.to_jit_type();

match (a, b) {
(JitValue::Int(a), JitValue::Int(b))
| (JitValue::Bool(a), JitValue::Bool(b))
| (JitValue::Bool(a), JitValue::Int(b))
| (JitValue::Int(a), JitValue::Bool(b)) => {
let operand_one = match a_type.unwrap() {
JitType::Bool => self.builder.ins().uextend(types::I64, a),
_ => a,
};

let operand_two = match b_type.unwrap() {
JitType::Bool => self.builder.ins().uextend(types::I64, b),
_ => b,
};

let cond = match op {
ComparisonOperator::Equal => IntCC::Equal,
ComparisonOperator::NotEqual => IntCC::NotEqual,
ComparisonOperator::Less => IntCC::SignedLessThan,
ComparisonOperator::LessOrEqual => IntCC::SignedLessThanOrEqual,
ComparisonOperator::Greater => IntCC::SignedGreaterThan,
ComparisonOperator::GreaterOrEqual => IntCC::SignedGreaterThanOrEqual,
};

let val = self.builder.ins().icmp(cond, operand_one, operand_two);
self.stack.push(JitValue::Bool(val));
Ok(())
}
(JitValue::Float(a), JitValue::Float(b)) => {
let cond = match op {
ComparisonOperator::Equal => FloatCC::Equal,
ComparisonOperator::NotEqual => FloatCC::NotEqual,
ComparisonOperator::Less => FloatCC::LessThan,
ComparisonOperator::LessOrEqual => FloatCC::LessThanOrEqual,
ComparisonOperator::Greater => FloatCC::GreaterThan,
ComparisonOperator::GreaterOrEqual => FloatCC::GreaterThanOrEqual,
};

let val = self.builder.ins().fcmp(cond, a, b);
self.stack.push(JitValue::Bool(val));
Ok(())
}
_ => Err(JitCompileError::NotSupported),
}
}
Instruction::UnaryOperation(op) => {
let op = op.get(arg);
let a = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
match (op, a) {
(UnaryOperator::Minus, JitValue::Int(val)) => {
// Compile minus as 0 - a.
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::BinaryOperation(op) | Instruction::BinaryOperationInplace(op) => {
let op = op.get(arg);
// the rhs is popped off first
Expand Down Expand Up @@ -561,26 +400,11 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {

Ok(())
}
Instruction::SetupLoop => {
let loop_head = self.builder.create_block();
self.builder.ins().jump(loop_head, &[]);
self.builder.switch_to_block(loop_head);
Ok(())
}
Instruction::PopBlock => {
// TODO: block support
Instruction::BuildTuple(size) => {
let elements = self.pop_multiple(size.get(arg) as usize);
self.stack.push(JitValue::Tuple(elements));
Ok(())
}
Instruction::LoadGlobal(idx) => {
let name = &bytecode.names[idx.get(arg) as usize];

if name.as_ref() != bytecode.obj_name.as_ref() {
Err(JitCompileError::NotSupported)
} else {
self.stack.push(JitValue::FuncRef(func_ref));
Ok(())
}
}
Instruction::CallFunctionPositional(nargs) => {
let nargs = nargs.get(arg);

Expand All @@ -601,20 +425,195 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
_ => Err(JitCompileError::BadBytecode),
}
}
Instruction::CompareOperation(op) => {
let op = op.get(arg);
// the rhs is popped off first
let b = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let a = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;

let a_type: Option<JitType> = a.to_jit_type();
let b_type: Option<JitType> = b.to_jit_type();

match (a, b) {
(JitValue::Int(a), JitValue::Int(b))
| (JitValue::Bool(a), JitValue::Bool(b))
| (JitValue::Bool(a), JitValue::Int(b))
| (JitValue::Int(a), JitValue::Bool(b)) => {
let operand_one = match a_type.unwrap() {
JitType::Bool => self.builder.ins().uextend(types::I64, a),
_ => a,
};

let operand_two = match b_type.unwrap() {
JitType::Bool => self.builder.ins().uextend(types::I64, b),
_ => b,
};

let cond = match op {
ComparisonOperator::Equal => IntCC::Equal,
ComparisonOperator::NotEqual => IntCC::NotEqual,
ComparisonOperator::Less => IntCC::SignedLessThan,
ComparisonOperator::LessOrEqual => IntCC::SignedLessThanOrEqual,
ComparisonOperator::Greater => IntCC::SignedGreaterThan,
ComparisonOperator::GreaterOrEqual => IntCC::SignedGreaterThanOrEqual,
};

let val = self.builder.ins().icmp(cond, operand_one, operand_two);
self.stack.push(JitValue::Bool(val));
Ok(())
}
(JitValue::Float(a), JitValue::Float(b)) => {
let cond = match op {
ComparisonOperator::Equal => FloatCC::Equal,
ComparisonOperator::NotEqual => FloatCC::NotEqual,
ComparisonOperator::Less => FloatCC::LessThan,
ComparisonOperator::LessOrEqual => FloatCC::LessThanOrEqual,
ComparisonOperator::Greater => FloatCC::GreaterThan,
ComparisonOperator::GreaterOrEqual => FloatCC::GreaterThanOrEqual,
};

let val = self.builder.ins().fcmp(cond, a, b);
self.stack.push(JitValue::Bool(val));
Ok(())
}
_ => Err(JitCompileError::NotSupported),
}
}
Instruction::ExtendedArg => Ok(()),
Instruction::Jump(target) => {
let target_block = self.get_or_create_block(target.get(arg));
self.builder.ins().jump(target_block, &[]);
Ok(())
}
Instruction::LoadConst { idx } => {
let val = self
.prepare_const(bytecode.constants[idx.get(arg) as usize].borrow_constant())?;
self.stack.push(val);
Ok(())
}
Instruction::LoadFast(idx) => {
let local = self.variables[idx.get(arg) as usize]
.as_ref()
.ok_or(JitCompileError::BadBytecode)?;
self.stack.push(JitValue::from_type_and_value(
local.ty.clone(),
self.builder.use_var(local.var),
));
Ok(())
}
Instruction::LoadGlobal(idx) => {
let name = &bytecode.names[idx.get(arg) as usize];

if name.as_ref() != bytecode.obj_name.as_ref() {
Err(JitCompileError::NotSupported)
} else {
self.stack.push(JitValue::FuncRef(func_ref));
Ok(())
}
}
Instruction::Nop => Ok(()),
Instruction::Pop => {
self.stack.pop();
Ok(())
}
Instruction::PopBlock => {
// TODO: block support
Ok(())
}
Instruction::PopJumpIfFalse(target) => {
let cond = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let val = self.boolean_val(cond)?;
let then_block = self.get_or_create_block(target.get(arg));
let else_block = self.builder.create_block();

self.builder
.ins()
.brif(val, else_block, &[], then_block, &[]);
self.builder.switch_to_block(else_block);

Ok(())
}
Instruction::PopJumpIfTrue(target) => {
let cond = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
let val = self.boolean_val(cond)?;
let then_block = self.get_or_create_block(target.get(arg));
let else_block = self.builder.create_block();

self.builder
.ins()
.brif(val, then_block, &[], else_block, &[]);
self.builder.switch_to_block(else_block);

Ok(())
}
Instruction::Resume(_resume_arg) => {
// TODO: Implement the resume instruction
Ok(())
}
Instruction::ReturnConst(idx) => {
let val = self
.prepare_const(bytecode.constants[idx.get(arg) as usize].borrow_constant())?;
self.return_value(val)
}
Instruction::ReturnValue => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.return_value(val)
}
Instruction::SetupLoop => {
let loop_head = self.builder.create_block();
self.builder.ins().jump(loop_head, &[]);
self.builder.switch_to_block(loop_head);
Ok(())
}
Instruction::StoreFast(idx) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.store_variable(idx.get(arg), val)
}
Instruction::Swap(index) => {
let len = self.stack.len();
let i = len - 1;
let j = len - 1 - index.get(arg) as usize;
self.stack.swap(i, j);
Ok(())
Comment on lines +568 to 577

@coderabbitai coderabbitai Bot Nov 29, 2025

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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the current Swap implementation and related code
cat -n crates/jit/src/instructions.rs | sed -n '565,580p'

Repository: RustPython/RustPython

Length of output: 186

🏁 Script executed:

# Search for compile.rs to see how SWAP is emitted
fd -t f "compile.rs" --exec cat -n {} +

Repository: RustPython/RustPython

Length of output: 186

🏁 Script executed:

# Search for tests related to Swap instruction
rg -n "Swap" crates/jit/src --type=rs -A 3 -B 3

Repository: RustPython/RustPython

Length of output: 186

🏁 Script executed:

# Look for any test files that exercise SWAP patterns
fd -t f -e rs --path "*/test*" | xargs rg -l "Swap\|SWAP" 2>/dev/null | head -20

Repository: RustPython/RustPython

Length of output: 186

🏁 Script executed:

# Check for any bytecode or stack trace comments explaining SWAP
rg -n "STACK\[" crates/jit/src --type=rs -A 2 -B 2

Repository: RustPython/RustPython

Length of output: 186

🌐 Web query:

CPython 3.12 SWAP bytecode instruction semantics documentation

💡 Result:

SWAP(i) in CPython 3.12 swaps the top-of-stack with the i‑th-from-top element:

STACK[-i], STACK[-1] = STACK[-1], STACK[-i]. [1][2]

(Opcode documented in the Python dis module; added in 3.11 and present in 3.12.) [1][2]

Sources:
[1] Python 3.12 dis — Disassembler for Python bytecode. [2] Python documentation (dis) (same opcode entry).

🌐 Web query:

Python dis SWAP instruction stack operations 1-based depth TOS

💡 Result:

SWAP(i) (added in CPython 3.11) swaps the top-of-stack (TOS) with the i‑th element below it, where i is 1‑based. Formally:

STACK[-i], STACK[-1] = STACK[-1], STACK[-i]. [1][2]

Example: stack = [A, B, C, D] (TOS = D)

  • SWAP(1) leaves it [A, B, C, D] (swap TOS with itself)
  • SWAP(2) → [A, B, D, C] (swap D and C)
  • SWAP(4) → [D, B, C, A] (swap D and A). [1][2]

Sources: Python dis module documentation. [1][2]

🌐 Web query:

CPython 3.12 augmented assignment s[a] += b bytecode SWAP pattern

💡 Result:

Short answer: CPython 3.12 uses two SWAPs to reorder the stack so STORE_SUBSCR sees (key, container, value) as it expects after the in-place BINARY_OP.

Why: BINARY_SUBSCR (earlier) leaves the stack as: ..., s, a, s[a]. After loading b and executing BINARY_OP (+=) the stack is ..., s, a, (s[a] + b). STORE_SUBSCR implements
key = STACK.pop()
container = STACK.pop()
value = STACK.pop()
container[key] = value
so the top three stack items must be (value, container, key) before STORE_SUBSCR runs. The two SWAP ops move the computed value down into the right position (first SWAP 3 moves the value below s and a; SWAP 2 then places key and container in the order STORE_SUBSCR expects). See the dis/bytecode semantics and an example disassembly of s[a] += b. [1][2]

Sources:

  • dis docs (BINARY_SUBSCR, BINARY_OP, STORE_SUBSCR semantics). [1]
  • Example disassembly and stack-walk explanation for s[a] += b showing the COPY/BINARY_SUBSCR → BINARY_OP → SWAP, SWAP → STORE_SUBSCR sequence. [2]

Swap instruction has a confirmed off‑by‑one error in JIT compilation

The web search confirms that CPython 3.12's SWAP(i) bytecode uses 1-based indexing and swaps STACK[-i] with STACK[-1] (the top of stack). For example, with stack [A, B, C, D] (TOS = D), SWAP(2) produces [A, B, D, C] by swapping D and C.

Tracing through the current implementation:

  • For SWAP(2) on a stack of length 4: i = 3, j = 4 - 1 - 2 = 1 → swaps stack[3] with stack[1] (D with B), yielding [A, D, C, B] ✗
  • Correct formula should be: j = len - index.get(arg) as usize → j = 4 - 2 = 2 → swaps stack[3] with stack[2] (D with C), yielding [A, B, D, C] ✓

This off-by-one error will cause incorrect stack reordering for any JIT-compiled code using Swap, including augmented assignment patterns like s[a] += b which use SWAP 3; SWAP 2 in CPython 3.12+.

Recommended fix:

- let j = len - 1 - index.get(arg) as usize;
+ let j = len - index.get(arg) as usize;

Add a unit test exercising SWAP 2 and SWAP 3 patterns to lock in correct semantics.

📝 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::StoreFast(idx) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.store_variable(idx.get(arg), val)
}
Instruction::Swap(index) => {
let len = self.stack.len();
let i = len - 1;
let j = len - 1 - index.get(arg) as usize;
self.stack.swap(i, j);
Ok(())
Instruction::StoreFast(idx) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
self.store_variable(idx.get(arg), val)
}
Instruction::Swap(index) => {
let len = self.stack.len();
let i = len - 1;
let j = len - index.get(arg) as usize;
self.stack.swap(i, j);
Ok(())
🤖 Prompt for AI Agents 
In crates/jit/src/instructions.rs around lines 568 to 577, the SWAP
implementation uses an off-by-one index calculation (j = len - 1 -
index.get(arg) as usize) which incorrectly treats the operand as 0-based; change
the calculation to use 1-based semantics (j = len - index.get(arg) as usize) so
SWAP(i) swaps STACK[-i] with STACK[-1], and update any comments accordingly;
also add unit tests exercising SWAP 2 and SWAP 3 (and combined patterns like
SWAP 3; SWAP 2 used in augmented assignment) to validate correct stack
reordering.

}
Instruction::Pop => {
self.stack.pop();
Ok(())
Instruction::UnaryOperation(op) => {
let op = op.get(arg);
let a = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
match (op, a) {
(UnaryOperator::Minus, JitValue::Int(val)) => {
// Compile minus as 0 - a.
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::Resume(_resume_arg) => {
// TODO: Implement the resume instruction
Instruction::UnpackSequence(size) => {
let val = self.stack.pop().ok_or(JitCompileError::BadBytecode)?;

let elements = match val {
JitValue::Tuple(elements) => elements,
_ => return Err(JitCompileError::NotSupported),
};

if elements.len() != size.get(arg) as usize {
return Err(JitCompileError::NotSupported);
}

self.stack.extend(elements.into_iter().rev());
Ok(())
}
_ => Err(JitCompileError::NotSupported),
Expand Down
Loading