Skip to content

Make logical operators short-circuit where possible #750

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

Open
triallax wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make logical operators short-circuit where possible #750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 28 additions & 3 deletions numbat/src/bytecode_interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,32 @@ impl BytecodeInterpreter {
self.compile_expression(lhs)?;
self.vm.add_op(Op::LogicalNeg);
}
Expression::BinaryOperator(
_span,
op @ (BinaryOperator::LogicalOr | BinaryOperator::LogicalAnd),
lhs,
rhs,
_type,
) => {
self.compile_expression(lhs)?;

let jump_offset = self.vm.current_offset() + 1; // +1 for the opcode
self.vm.add_op1(
match op {
BinaryOperator::LogicalOr => Op::JumpIfTrue,
BinaryOperator::LogicalAnd => Op::JumpIfFalse,
_ => unreachable!(),
},
0xffff,
);
self.vm.add_op(Op::Pop);

self.compile_expression(rhs)?;

let else_block_offset = self.vm.current_offset();
self.vm
.patch_u16_value_at(jump_offset, else_block_offset - (jump_offset + 2));
}
Expression::BinaryOperator(_span, operator, lhs, rhs, _type) => {
self.compile_expression(lhs)?;
self.compile_expression(rhs)?;
Expand All @@ -126,8 +152,7 @@ impl BytecodeInterpreter {
BinaryOperator::GreaterOrEqual => Op::GreatorOrEqual,
BinaryOperator::Equal => Op::Equal,
BinaryOperator::NotEqual => Op::NotEqual,
BinaryOperator::LogicalAnd => Op::LogicalAnd,
BinaryOperator::LogicalOr => Op::LogicalOr,
BinaryOperator::LogicalAnd | BinaryOperator::LogicalOr => unreachable!(),
};
self.vm.add_op(op);
}
Expand Down Expand Up @@ -243,7 +268,7 @@ impl BytecodeInterpreter {
self.compile_expression(condition)?;

let if_jump_offset = self.vm.current_offset() + 1; // +1 for the opcode
self.vm.add_op1(Op::JumpIfFalse, 0xffff);
self.vm.add_op1(Op::PopJumpIfFalse, 0xffff);

self.compile_expression(then_expr)?;

Expand Down
4 changes: 2 additions & 2 deletions numbat/src/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ impl Value {
}

#[track_caller]
pub fn unsafe_as_bool(self) -> bool {
pub fn unsafe_as_bool(&self) -> bool {
if let Value::Boolean(b) = self {
b
*b
} else {
panic!("Expected value to be a bool");
}
Expand Down
59 changes: 40 additions & 19 deletions numbat/src/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,11 @@ pub enum Op {
GreatorOrEqual,
Equal,
NotEqual,
LogicalAnd,
LogicalOr,
LogicalNeg,

/// Pop the top value off the stack.
Pop,

/// Similar to Add, but has DateTime on the LHS and a quantity on the RHS
AddToDateTime,
/// Similar to Sub, but has DateTime on the LHS and a quantity on the RHS
Expand All @@ -88,7 +89,13 @@ pub enum Op {

/// Move IP forward by the given offset argument if the popped-of value on
/// top of the stack is false.
PopJumpIfFalse,
/// Move IP forward by the given offset argument if the peeked value on
/// top of the stack is false.
JumpIfFalse,
/// Move IP forward by the given offset argument if the peeked value on
/// top of the stack is true.
JumpIfTrue,
/// Unconditionally move IP forward by the given offset argument
Jump,

Expand Down Expand Up @@ -132,7 +139,9 @@ impl Op {
| Op::GetUpvalue
| Op::PrintString
| Op::JoinString
| Op::PopJumpIfFalse
| Op::JumpIfFalse
| Op::JumpIfTrue
| Op::Jump
| Op::CallCallable
| Op::AccessStructField
Expand All @@ -154,11 +163,10 @@ impl Op {
| Op::GreatorOrEqual
| Op::Equal
| Op::NotEqual
| Op::LogicalAnd
| Op::LogicalOr
| Op::LogicalNeg
| Op::Return
| Op::GetLastResult => 0,
| Op::GetLastResult
| Op::Pop => 0,
}
}

Expand Down Expand Up @@ -187,10 +195,10 @@ impl Op {
Op::GreatorOrEqual => "GreatorOrEqual",
Op::Equal => "Equal",
Op::NotEqual => "NotEqual",
Op::LogicalAnd => "LogicalAnd",
Op::LogicalOr => "LogicalOr",
Op::LogicalNeg => "LogicalNeg",
Op::PopJumpIfFalse => "PopJumpIfFalse",
Op::JumpIfFalse => "JumpIfFalse",
Op::JumpIfTrue => "JumpIfTrue",
Op::Jump => "Jump",
Op::Call => "Call",
Op::FFICallFunction => "FFICallFunction",
Expand All @@ -202,6 +210,7 @@ impl Op {
Op::BuildStructInstance => "BuildStructInstance",
Op::AccessStructField => "AccessStructField",
Op::BuildList => "BuildList",
Op::Pop => "Pop",
}
}
}
Expand Down Expand Up @@ -584,6 +593,14 @@ impl Vm {
self.pop().unsafe_as_bool()
}

#[track_caller]
fn peek_bool(&mut self) -> bool {
self.stack
.last()
.expect("stack should not be empty")
.unsafe_as_bool()
}

#[track_caller]
fn pop_datetime(&mut self) -> jiff::Zoned {
match self.pop() {
Expand Down Expand Up @@ -634,6 +651,9 @@ impl Vm {
self.stack
.push(self.constants[constant_idx as usize].to_value());
}
Op::Pop => {
self.pop();
}
Op::ApplyPrefix => {
let quantity = self.pop_quantity();
let prefix_idx = self.read_u16();
Expand Down Expand Up @@ -788,17 +808,6 @@ impl Vm {
};
self.push(Value::Boolean(result));
}
op @ (Op::LogicalAnd | Op::LogicalOr) => {
let rhs = self.pop_bool();
let lhs = self.pop_bool();

let result = match op {
Op::LogicalAnd => lhs && rhs,
Op::LogicalOr => lhs || rhs,
_ => unreachable!(),
};
self.push_bool(result);
}
Op::LogicalNeg => {
let rhs = self.pop_bool();
self.push_bool(!rhs);
Expand All @@ -824,12 +833,24 @@ impl Vm {

self.push_quantity(Quantity::from_scalar(math::factorial(lhs, order)));
}
Op::JumpIfFalse => {
Op::PopJumpIfFalse => {
let offset = self.read_u16() as usize;
if !self.pop_bool() {
self.current_frame_mut().ip += offset;
}
}
Op::JumpIfFalse => {
let offset = self.read_u16() as usize;
if !self.peek_bool() {
self.current_frame_mut().ip += offset;
}
}
Op::JumpIfTrue => {
let offset = self.read_u16() as usize;
if self.peek_bool() {
self.current_frame_mut().ip += offset;
}
}
Op::Jump => {
let offset = self.read_u16() as usize;
self.current_frame_mut().ip += offset;
Expand Down
6 changes: 6 additions & 0 deletions numbat/tests/interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,12 @@ fn test_logical() {
expect_output("true && false", "false");
expect_output("true && true", "true");

// short-circuiting
expect_output("false && head([])", "false");
expect_output("true || head([])", "true");
insta::assert_snapshot!(fail("true && head([])"), @"Empty list");
insta::assert_snapshot!(fail("false || head([])"), @"Empty list");

// priority
expect_output("false || true && false", "false");
expect_output("false || true && !false", "true");
Expand Down