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

Remove constraints blocks #652

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Remove constraints blocks #652

merged 1 commit into from
Oct 4, 2023

Conversation

Schaeff
Copy link
Collaborator

@Schaeff Schaeff commented Oct 2, 2023

machine {
   constraints {
      col witness x;
   }
   constraints {
      col witness y;
      x = y;
   }
}

Becomes

machine {
   col witness x;
   col witness y;
   x = y;
}

I did not change analysis/README.md as the diff looks quite different now.
I also tried to update the book in a way that made sense, pointing to the docs for powdr-pil when mentioning pil code in machines.

@Schaeff Schaeff force-pushed the remove-constraints-blocks branch from 3500770 to 8af2582 Compare October 2, 2023 13:08
*body = self.expand_macros(std::mem::take(body))
}
InstructionBody::CallableRef(..) => {
machine.statements = machine
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was changed to a functional approach, as a statement can yield many, and iter_mut does not allow that. Hence the usage of flat_map here.

_ => {}
});
MachineStatement::Pil(start, statement) => self
.expand_macros(vec![statement])
Copy link
Collaborator Author

@Schaeff Schaeff Oct 2, 2023

Choose a reason for hiding this comment

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

It would make more sense to me for expand_macros to take a single statement rather than a list, but I tried to keep this diff small.

@@ -40,7 +40,7 @@ impl<T: Display> Display for Machine<T> {
write_items_indented(f, &self.registers)?;
write_items_indented(f, &self.instructions)?;
write_items_indented(f, &self.callable)?;
write_items_indented(f, &self.constraints)?;
write_items_indented(f, &self.pil)?;
Copy link
Member

Choose a reason for hiding this comment

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

Is it a problem that these might be ordered differently? I mean can we always re-parse stuff that is printed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate here? Within constraints, the order is preserved throughout the compiler, but eventually this should not be a requirement, right? There are no tests which parse outputs of this display afaik, this could make sense to add but it seems unrelated to this change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just wondering if it might make sense to move the pil a bit further up or what happens if you mix constraints and instruction declarations and such, but yes, unrelated to this PR.

chriseth
chriseth previously approved these changes Oct 2, 2023
@Schaeff Schaeff force-pushed the remove-constraints-blocks branch 2 times, most recently from fa493ad to 71b1dc1 Compare October 3, 2023 15:38
@Schaeff Schaeff enabled auto-merge October 3, 2023 16:30
@Schaeff Schaeff requested a review from chriseth October 3, 2023 16:30
@Schaeff Schaeff force-pushed the remove-constraints-blocks branch from 71b1dc1 to 0950ff5 Compare October 4, 2023 10:35
@chriseth
Copy link
Member

chriseth commented Oct 4, 2023

still conflicts.

@Schaeff Schaeff force-pushed the remove-constraints-blocks branch from 0950ff5 to a3ebf46 Compare October 4, 2023 10:53
@Schaeff Schaeff added this pull request to the merge queue Oct 4, 2023
Merged via the queue into main with commit 12e94ac Oct 4, 2023
@Schaeff Schaeff deleted the remove-constraints-blocks branch October 4, 2023 12:45
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