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

Refactor hash chiplet constants and chiplets trace tests module #344

Merged
merged 4 commits into from
Aug 3, 2022

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Aug 2, 2022

This PR is a precursor to the PR that will complete the inclusion of the hasher lookups in the b_chip bus column. It includes the following refactors:

  • removes the re-export of the hasher module at the top level of core and switches imports to go through core::chiplets::hasher (rather than core::hasher)
  • moves the index constants for identifying hasher columns within the main trace from air to core
  • moves b_chip tests for memory and bitwise lookups into their own files

@grjte grjte marked this pull request as ready for review August 2, 2022 10:32
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a minor comment inline - but that could be done in a future PR.

@@ -257,7 +257,7 @@ pub fn parse_storew(
/// values (represented by 32-bit limbs), divides one value by another, and injects the quotient
/// and the remainder into the advice tape.
pub fn parse_adv_inject(
span_ops: &mut Vec<Operation>,
span_ops: &mut [Operation],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment above needs to be updated as well - but could be done in a different PR (maybe just leave a TODO for now?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what's needed here - I was just fixing a clippy warning. Is the issue with the comment that the code is actually appending to the decorators rather than to span_ops? I'm not going to leave a TODO, as I'm not sure what to put, but I'm happy to open an issue once I understand what's needed. For now I'm going to merge this PR so I can move forward to wrap up the b_chip column.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Yes, the changes are that we are appending to decorators, and actually span_ops does not need to be mutable here.

@grjte grjte merged commit fcad025 into 0xPolygonMiden:next Aug 3, 2022
@grjte grjte deleted the refactor-chiplets branch August 3, 2022 09:40
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.

None yet

2 participants