-
Notifications
You must be signed in to change notification settings - Fork 36
Allow support for computed indices #444
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
Changes from 22 commits
c56df8f
426ffd2
124ca12
5456c4b
00df247
b3bc934
52dbd20
2891862
1202dec
6634e61
f9f7689
d4a07ab
8af9c70
62f6ddd
a150733
9b01f26
b27ec35
e63ae1a
2b0de7d
85d4dc2
b40cca5
88bb70c
6e734dc
90b5bdc
abbf525
c8adbf5
82cd993
770b6b6
c48bfd4
a988ae5
d7b557f
02610a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| def ComputedIndicesAir | ||
|
|
||
| const MDS = [ | ||
| [1, 2], | ||
| [2, 3], | ||
| [3, 4] | ||
| ]; | ||
|
|
||
| trace_columns { | ||
| main: [a[2]], | ||
| } | ||
|
|
||
| public_inputs { | ||
| input: [1], | ||
| } | ||
|
|
||
| fn double(a: felt) -> felt { | ||
| let x = 3 * a; | ||
| let y = a; | ||
| return x - y; | ||
| } | ||
|
|
||
| boundary_constraints { | ||
| enf a[0].first = 0; | ||
| } | ||
|
|
||
| integrity_constraints { | ||
| let state_1 = [1, 1]; | ||
| let state_2 = [2, 0]; | ||
|
|
||
| # [1+2, 2+3, 3+4] | ||
| # [3, 5, 7] | ||
| let vec_1 = apply_mds(state_1); | ||
|
|
||
| # [2, 4, 6] | ||
| let vec_2 = apply_mds(state_2); | ||
|
|
||
| # 2 * 2 - 4 | ||
| let x = double(2) - vec_2[1]; | ||
|
|
||
| # vec_2[0] = 2 | ||
| let y = vec_2[x]; | ||
|
|
||
| # vec_1[2] = 7 | ||
| let z = vec_1[y]; | ||
|
|
||
| enf a[1] = double(z); | ||
| } | ||
|
|
||
| fn apply_mds(state: felt[2]) -> felt[3] { | ||
| return [sum([s * m for (s, m) in (state, mds_row)]) for mds_row in MDS]; | ||
| } | ||
|
Comment on lines
+59
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the part that was not possible before, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may not be the right place, but I imagine this is actually the only place where we'd ever use matrices. For the rare cases where we want to evaluate a matrix (only hash functions I think), we could just manually implement it. If so it might be worth removing matrices entirely. WDYT?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes when coupled with a function. I think it was ok without a function before because it was computed before semantic analysis.
I agree, its already in the todo list for the frontend refactor. Ill gather those in a separate issue.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not exactly, the apply_mds function was already tested in https://github.com/0xMiden/air-script/blob/allow-computed-indices-upstream/air-script/tests/list_comprehension/list_comprehension_nested.air What is new is to be able to index into the In this test (computed_indices_complex), I needed a vector that would not be folded as a constant during AST's constant propagation, otherwise I would not be able to test the computed indices targetting list comprehensions properly. I'll add some comments to make what this test does clearer. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe simplify this test and at the same time make it a bit more robust. Specifically:
state.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adapted the air-script of the test in 82cd993 but it does not compile as is, I'm investigating why that is.
It seems we extract the constant from the wrong value, extracting the
vec_1 instead of 1or report the error incorrectly here:air-script/air-script/tests/computed_indices/computed_indices_complex.air
Line 39 in 82cd993
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest commit, I've made it so that one MDS is kept on a constant vector to be able to produce constant indices, and use it to index into the second MDS that does rely on the state.
I've also added comments on what is expected / happening in the test, please tell me if it's better!