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

Write chiplet module constraints in AirScript #253

Merged
merged 4 commits into from
May 30, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Apr 13, 2023

Addressing: #201
This PR adds constraints for the chiplet module.

Copy link
Contributor

@grjte grjte 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 @Overcastan! This looks correct, but I think we can actually do something much simpler, as I proposed inline

constraints/miden-vm/chiplets.air Outdated Show resolved Hide resolved
constraints/miden-vm/chiplets.air Outdated Show resolved Hide resolved
constraints/miden-vm/chiplets.air Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-chiplets-module-constraints branch from 4a479d8 to 426330e Compare May 2, 2023 12:51
@Fumuran Fumuran requested review from grjte and tohrnii May 2, 2023 12:53
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

It all looks correct now @Overcastan - I would just simplify a bit more and do some minor renaming as noted inline

Comment on lines 16 to 18
hash_chiplet([s[1], s[2], chip_columns]) when !s[0]
bitwise_chiplet([s[2], chip_columns]) when s[0] & !s[1]
memory_chiplet([chip_columns]) when s[0] & s[1] & !s[2]'
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be chiplet_columns rather than chip_columns.

Also, I think this is simple enough now that we can just move this to the main chiplets evaluator and remove this helper evaluator


ev chiplets_constraints(main: [s[3], chiplet_columns[15]]):
enf chiplet_selector_constraints([s])

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would remove this empty line

### Chiplets Constraints ##########################################################################

ev chiplets_constraints(main: [s[3], chiplet_columns[15]]):
enf chiplet_selector_constraints([s])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would do a bit of renaming for more consistency with our other files

  1. chiplet_selector_constraints -> chiplet_selectors
  2. chiplet_constraints -> deleted
  3. chiplets_constraints -> chiplets

@Fumuran Fumuran requested a review from grjte May 2, 2023 17:39
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Thanks @Overcastan, there's an error I just noticed, otherwise this is nearly ready.

constraints/miden-vm/chiplets.air Outdated Show resolved Hide resolved

### Chiplets Constraints ##########################################################################

ev chiplets(main: [s[3], chiplet_columns[15]]):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be good to have a comment here.

@Fumuran Fumuran requested a review from grjte May 3, 2023 11:52
Copy link
Contributor

@grjte grjte 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!

@grjte grjte merged commit e1cb1c3 into next May 30, 2023
@grjte grjte deleted the andrew-chiplets-module-constraints branch May 30, 2023 15:09
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.

write MidenVM constraints for chiplets module (excludes multiset check constraints)
3 participants