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.

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

Add support for boundary variables #160

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Add support for boundary variables #160

merged 2 commits into from
Feb 22, 2023

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Feb 21, 2023

This PR addresses #106. As mentioned in #150, I'd like to do some refactoring, including the symbol table, so I consider this implementation to be a preliminary version

- add variable scope for BoundaryConstraints and IntegrityConstraints
Comment on lines +294 to +316
// The scope of the variable must be valid for the constraint domain.
match (scope, domain) {
(
Scope::BoundaryConstraints,
ConstraintDomain::FirstRow | ConstraintDomain::LastRow,
)
| (
Scope::IntegrityConstraints,
ConstraintDomain::EveryRow | ConstraintDomain::EveryFrame(_),
) => {
let key = (scope, variable_value);
if let Some(expr) = variable_roots.get(&key) {
match scope {
Scope::BoundaryConstraints => {
// TODO: deal with boundary conflict properly
Ok(ExprDetails::new(
expr.root_idx(),
expr.trace_segment(),
domain,
))
}
Scope::IntegrityConstraints => Ok(*expr),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be addressed during #150, but it actually requires significant refactoring to do it cleanly

Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

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

Thank you @grjte, looks great!

Comment on lines +190 to +195
integrity_constraints:
let a = 1
enf clk' = clk + 1
boundary_constraints:
enf clk.first = 0
enf clk.last = a";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to reverse the order of integrity and boundary constraints sections here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, no! Good catch, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, yes - it is. It needs to be reversed there in order to test defining the variable in the integrity_constraints and then using it in the boundary_constraints (out of scope)

Copy link
Contributor

@tohrnii tohrnii Feb 21, 2023

Choose a reason for hiding this comment

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

My mistake, I thought that it would work the other way around as well and you probably did it to make the example clearer but it wouldn't work the other way around.

@grjte grjte merged commit b8641d9 into next Feb 22, 2023
@grjte grjte deleted the grjte-ir-bc-variables branch February 22, 2023 08:52
@tohrnii tohrnii mentioned this pull request Feb 22, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants