Skip to content

Commit b704f42

Browse files
ShaharNavehShaharNaveh
authored
Reduce usage JumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare (#6524)
* Remove `JumpIfTrueOrPop` & `JumpIfFalseOrPop` opcdes * Use correct instruction name * Extract `compile_cmpop` to its own method * Better alignment with CPython code * Restore `PopJumpIf` instructions * Revert PopJumpIfFalse for comparisons
1 parent 7f4d308 commit b704f42

File tree

1 file changed

+80
-69
lines changed

1 file changed

+80
-69
lines changed

‎crates/codegen/src/compile.rs‎

Lines changed: 80 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -4173,7 +4173,9 @@ impl Compiler {
41734173
if let Some(ref guard) = m.guard {
41744174
// Compile guard and jump to end if false
41754175
self.compile_expression(guard)?;
4176-
emit!(self, Instruction::JumpIfFalseOrPop { target: end });
4176+
emit!(self, Instruction::CopyItem { index: 1_u32 });
4177+
emit!(self, Instruction::PopJumpIfFalse { target: end });
4178+
emit!(self, Instruction::PopTop);
41774179
}
41784180
self.compile_statements(&m.body)?;
41794181
}
@@ -4187,92 +4189,97 @@ impl Compiler {
41874189
Ok(())
41884190
}
41894191

4190-
fn compile_chained_comparison(
4192+
/// [CPython `compiler_addcompare`](https://github.com/python/cpython/blob/627894459a84be3488a1789919679c997056a03c/Python/compile.c#L2880-L2924)
4193+
fn compile_addcompare(&mut self, op: &CmpOp) {
4194+
use bytecode::ComparisonOperator::*;
4195+
match op {
4196+
CmpOp::Eq => emit!(self, Instruction::CompareOperation { op: Equal }),
4197+
CmpOp::NotEq => emit!(self, Instruction::CompareOperation { op: NotEqual }),
4198+
CmpOp::Lt => emit!(self, Instruction::CompareOperation { op: Less }),
4199+
CmpOp::LtE => emit!(self, Instruction::CompareOperation { op: LessOrEqual }),
4200+
CmpOp::Gt => emit!(self, Instruction::CompareOperation { op: Greater }),
4201+
CmpOp::GtE => {
4202+
emit!(self, Instruction::CompareOperation { op: GreaterOrEqual })
4203+
}
4204+
CmpOp::In => emit!(self, Instruction::ContainsOp(Invert::No)),
4205+
CmpOp::NotIn => emit!(self, Instruction::ContainsOp(Invert::Yes)),
4206+
CmpOp::Is => emit!(self, Instruction::IsOp(Invert::No)),
4207+
CmpOp::IsNot => emit!(self, Instruction::IsOp(Invert::Yes)),
4208+
}
4209+
}
4210+
4211+
/// Compile a chained comparison.
4212+
///
4213+
/// ```py
4214+
/// a == b == c == d
4215+
/// ```
4216+
///
4217+
/// Will compile into (pseudo code):
4218+
///
4219+
/// ```py
4220+
/// result = a == b
4221+
/// if result:
4222+
/// result = b == c
4223+
/// if result:
4224+
/// result = c == d
4225+
/// ```
4226+
///
4227+
/// # See Also
4228+
/// - [CPython `compiler_compare`](https://github.com/python/cpython/blob/627894459a84be3488a1789919679c997056a03c/Python/compile.c#L4678-L4717)
4229+
fn compile_compare(
41914230
&mut self,
41924231
left: &Expr,
41934232
ops: &[CmpOp],
4194-
exprs: &[Expr],
4233+
comparators: &[Expr],
41954234
) -> CompileResult<()> {
4196-
assert!(!ops.is_empty());
4197-
assert_eq!(exprs.len(), ops.len());
41984235
let (last_op, mid_ops) = ops.split_last().unwrap();
4199-
let (last_val, mid_exprs) = exprs.split_last().unwrap();
4200-
4201-
use bytecode::ComparisonOperator::*;
4202-
let compile_cmpop = |c: &mut Self, op: &CmpOp| match op {
4203-
CmpOp::Eq => emit!(c, Instruction::CompareOperation { op: Equal }),
4204-
CmpOp::NotEq => emit!(c, Instruction::CompareOperation { op: NotEqual }),
4205-
CmpOp::Lt => emit!(c, Instruction::CompareOperation { op: Less }),
4206-
CmpOp::LtE => emit!(c, Instruction::CompareOperation { op: LessOrEqual }),
4207-
CmpOp::Gt => emit!(c, Instruction::CompareOperation { op: Greater }),
4208-
CmpOp::GtE => {
4209-
emit!(c, Instruction::CompareOperation { op: GreaterOrEqual })
4210-
}
4211-
CmpOp::In => emit!(c, Instruction::ContainsOp(Invert::No)),
4212-
CmpOp::NotIn => emit!(c, Instruction::ContainsOp(Invert::Yes)),
4213-
CmpOp::Is => emit!(c, Instruction::IsOp(Invert::No)),
4214-
CmpOp::IsNot => emit!(c, Instruction::IsOp(Invert::Yes)),
4215-
};
4216-
4217-
// a == b == c == d
4218-
// compile into (pseudo code):
4219-
// result = a == b
4220-
// if result:
4221-
// result = b == c
4222-
// if result:
4223-
// result = c == d
4236+
let (last_comparator, mid_comparators) = comparators.split_last().unwrap();
42244237

42254238
// initialize lhs outside of loop
42264239
self.compile_expression(left)?;
42274240

4228-
let end_blocks = if mid_exprs.is_empty() {
4229-
None
4230-
} else {
4231-
let break_block = self.new_block();
4232-
let after_block = self.new_block();
4233-
Some((break_block, after_block))
4234-
};
4241+
if mid_comparators.is_empty() {
4242+
self.compile_expression(last_comparator)?;
4243+
self.compile_addcompare(last_op);
4244+
4245+
return Ok(());
4246+
}
4247+
4248+
let cleanup = self.new_block();
42354249

42364250
// for all comparisons except the last (as the last one doesn't need a conditional jump)
4237-
for (op, val) in mid_ops.iter().zip(mid_exprs) {
4238-
self.compile_expression(val)?;
4251+
for (op, comparator) in mid_ops.iter().zip(mid_comparators) {
4252+
self.compile_expression(comparator)?;
4253+
42394254
// store rhs for the next comparison in chain
42404255
emit!(self, Instruction::Swap { index: 2 });
4241-
emit!(self, Instruction::CopyItem { index: 2_u32 });
4256+
emit!(self, Instruction::CopyItem { index: 2 });
42424257

4243-
compile_cmpop(self, op);
4258+
self.compile_addcompare(op);
42444259

42454260
// if comparison result is false, we break with this value; if true, try the next one.
4246-
if let Some((break_block, _)) = end_blocks {
4247-
emit!(
4248-
self,
4249-
Instruction::JumpIfFalseOrPop {
4250-
target: break_block,
4251-
}
4252-
);
4253-
}
4261+
/*
4262+
emit!(self, Instruction::CopyItem { index: 1 });
4263+
// emit!(self, Instruction::ToBool); // TODO: Uncomment this
4264+
emit!(self, Instruction::PopJumpIfFalse { target: cleanup });
4265+
emit!(self, Instruction::PopTop);
4266+
*/
4267+
4268+
emit!(self, Instruction::JumpIfFalseOrPop { target: cleanup });
42544269
}
42554270

4256-
// handle the last comparison
4257-
self.compile_expression(last_val)?;
4258-
compile_cmpop(self, last_op);
4271+
self.compile_expression(last_comparator)?;
4272+
self.compile_addcompare(last_op);
42594273

4260-
if let Some((break_block, after_block)) = end_blocks {
4261-
emit!(
4262-
self,
4263-
Instruction::Jump {
4264-
target: after_block,
4265-
}
4266-
);
4267-
4268-
// early exit left us with stack: `rhs, comparison_result`. We need to clean up rhs.
4269-
self.switch_to_block(break_block);
4270-
emit!(self, Instruction::Swap { index: 2 });
4271-
emit!(self, Instruction::PopTop);
4274+
let end = self.new_block();
4275+
emit!(self, Instruction::Jump { target: end });
42724276

4273-
self.switch_to_block(after_block);
4274-
}
4277+
// early exit left us with stack: `rhs, comparison_result`. We need to clean up rhs.
4278+
self.switch_to_block(cleanup);
4279+
emit!(self, Instruction::Swap { index: 2 });
4280+
emit!(self, Instruction::PopTop);
42754281

4282+
self.switch_to_block(end);
42764283
Ok(())
42774284
}
42784285

@@ -4600,27 +4607,31 @@ impl Compiler {
46004607
let after_block = self.new_block();
46014608

46024609
let (last_value, values) = values.split_last().unwrap();
4610+
46034611
for value in values {
46044612
self.compile_expression(value)?;
46054613

4614+
emit!(self, Instruction::CopyItem { index: 1_u32 });
46064615
match op {
46074616
BoolOp::And => {
46084617
emit!(
46094618
self,
4610-
Instruction::JumpIfFalseOrPop {
4619+
Instruction::PopJumpIfFalse {
46114620
target: after_block,
46124621
}
46134622
);
46144623
}
46154624
BoolOp::Or => {
46164625
emit!(
46174626
self,
4618-
Instruction::JumpIfTrueOrPop {
4627+
Instruction::PopJumpIfTrue {
46194628
target: after_block,
46204629
}
46214630
);
46224631
}
46234632
}
4633+
4634+
emit!(self, Instruction::PopTop);
46244635
}
46254636

46264637
// If all values did not qualify, take the value of the last value:
@@ -4697,7 +4708,7 @@ impl Compiler {
46974708
comparators,
46984709
..
46994710
}) => {
4700-
self.compile_chained_comparison(left, ops, comparators)?;
4711+
self.compile_compare(left, ops, comparators)?;
47014712
}
47024713
// Expr::Constant(ExprConstant { value, .. }) => {
47034714
// self.emit_load_const(compile_constant(value));

0 commit comments

Comments
 (0)