Skip to content
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

wip: use arena-style cache when simplifying Expressions (DO NOT MERGE) #276

Draft
wants to merge 6 commits into
base: simplify-by-hand
Choose a base branch
from
Draft

wip: use arena-style cache when simplifying Expressions (DO NOT MERGE) #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c6dd84
chore: test that similar calibrations with different sites are not fi…
MarquessV Aug 3, 2023
0f95a7a
Merge remote-tracking branch 'origin/simplify-by-hand' into expressio…
Shadow53 Aug 4, 2023
b583c36
refactor: add a simplification cache
Shadow53 Aug 5, 2023
a3dce2d
refactor!: WIP simplify expressions by value
Shadow53 Aug 7, 2023
3993374
feat(lib): take ownership of Expression during simplification
Shadow53 Aug 9, 2023
e3c084a
refactor: avoid hashing in Expression PartialEq
Shadow53 Aug 9, 2023
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
39 changes: 32 additions & 7 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion quil-rs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ categories = ["parser-implementations", "science", "compilers", "emulators"]
approx = { version = "0.5.1", features = ["num-complex"] }
dot-writer = { version = "0.1.2", optional = true }
egg = { version = "0.9.4", features = ["deterministic"] }
generational-arena = "0.2.9"
indexmap = "1.6.1"
itertools = "0.11.0"
lexical = "6.1.1"
Expand All @@ -33,7 +34,7 @@ insta = "1.7.1"
petgraph = "0.6.2"
proptest = "1.0.0"
proptest-derive = "0.3.0"
rstest = "0.17.0"
rstest = "0.18.1"

# These are described in the crate README.md
[features]
Expand Down
43 changes: 26 additions & 17 deletions quil-rs/src/expression/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use std::{
#[cfg(test)]
use proptest_derive::Arbitrary;

mod simplification;
//mod simplification;
mod simplification_2;

/// The different possible types of errors that could occur during expression evaluation.
Expand All @@ -60,7 +60,7 @@ pub enum Expression {
Variable(String),
}

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct FunctionCallExpression {
pub function: ExpressionFunction,
pub expression: Box<Expression>,
Expand All @@ -75,7 +75,7 @@ impl FunctionCallExpression {
}
}

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct InfixExpression {
pub left: Box<Expression>,
pub operator: InfixOperator,
Expand All @@ -92,7 +92,7 @@ impl InfixExpression {
}
}

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct PrefixExpression {
pub operator: PrefixOperator,
pub expression: Box<Expression>,
Expand Down Expand Up @@ -135,15 +135,15 @@ impl Hash for Expression {
"Infix".hash(state);
operator.hash(state);
match operator {
InfixOperator::Plus | InfixOperator::Star => {
// commutative, so put left & right in decreasing order by hash value
let (a, b) = (
min_by_key(&left, &right, hash_to_u64),
max_by_key(&left, &right, hash_to_u64),
);
a.hash(state);
b.hash(state);
}
//InfixOperator::Plus | InfixOperator::Star => {
// // commutative, so put left & right in decreasing order by hash value
Copy link
Contributor

@genos genos Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that this is costly, but we put it in for #27. Are we deciding to no longer ensure that, e.g., 1 + x == x + 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, should we just derive Hash, PartialEq, and Eq and call it a day?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I think this is the way to go. #27 only handles commutativity, but not other rules that lead to equality between expressions. Since we're simplifying expressions with these rules, enforcing some but not all in the hashing & equality implementations seems wrong and wasteful. I'll open a new PR.

// let (a, b) = (
// min_by_key(&left, &right, hash_to_u64),
// max_by_key(&left, &right, hash_to_u64),
// );
// a.hash(state);
// b.hash(state);
//}
_ => {
left.hash(state);
right.hash(state);
Expand Down Expand Up @@ -181,7 +181,17 @@ impl Hash for Expression {
impl PartialEq for Expression {
// Partial equality by hash value
fn eq(&self, other: &Self) -> bool {
hash_to_u64(self) == hash_to_u64(other)
// hash_to_u64(self) == hash_to_u64(other)
match (self, other) {
(Self::Address(left), Self::Address(right)) => left == right,
(Self::Infix(left), Self::Infix(right)) => left == right,
(Self::Number(left), Self::Number(right)) => left == right,
(Self::Prefix(left), Self::Prefix(right)) => left == right,
(Self::FunctionCall(left), Self::FunctionCall(right)) => left == right,
(Self::Variable(left), Self::Variable(right)) => left == right,
(Self::PiConstant, Self::PiConstant) => true,
_ => false,
}
}
}

Expand Down Expand Up @@ -274,9 +284,8 @@ impl Expression {
*self = Expression::Number(Complex64::from(PI));
}
_ => {
if let Ok(simpler) = simplification::run(self) {
*self = simpler;
}
let temp = std::mem::replace(self, Expression::PiConstant);
Copy link
Contributor

Choose a reason for hiding this comment

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

I really need to remember this trick 👍

*self = simplification_2::run(temp);
}
}
}
Expand Down
Loading