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 Memory chiplet bus constraints in AirScript #263

Merged
merged 2 commits into from
May 30, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Apr 21, 2023

Partially addressing: #203
This PR adds memory chiplet bus constraints and removes keywords from evaluator function declaration.

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! I think this looks correct. I just left a few minor nits inline

Comment on lines 22 to 18
ev is_zero(main: [values[4]]):
ev is_zero([values[4]]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this to be an evaluator that acts on a single column like the way we've done is_binary. The comprehension would then be at level where the evaluator is called. I think this would be clearer and more consistent with our approach in other places. (That way, if we move all of these into a utils module in the future we could use the same is_zero, is_unchanged, is_binary, etc. in all modules)

Comment on lines 28 to 24
ev is_unchanged(main: [values[4]]):
ev is_unchanged([values[4]]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my previous comment - I would prefer this to be an evaluator that acts on a single column like the way we've done is_binary. The comprehension would then be at level where the evaluator is called. I think this would be clearer and more consistent with our approach in other places. (That way, if we move all of these into a utils module in the future we could use the same is_zero, is_unchanged, is_binary, etc. in all modules)

@@ -107,10 +103,22 @@ ev enforce_values(main: [s[2], v[4]]):
enf is_unchanged([v]) when s[1]


ev bus_constraints([s[2], ctx, addr, clk, v[4]], [b_chip]):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's just call this chiplet_bus

Comment on lines 107 to 108
# Calculate operation label for each operation
let op_label = s[0] * 2^3 + 4
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's rename this to "op_mem" like the docs and change the comment to "Calculate the operation label for the memory access operation"

@Fumuran Fumuran requested a review from grjte May 3, 2023 19:21
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 273a8d0 into next May 30, 2023
@grjte grjte deleted the andrew-memory-range-check branch May 30, 2023 15:14
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