-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
c56df8f
feat(parser): Allow computed indices in AST
Leo-Besancon 426ffd2
feat(mir): Allow computed indices in MIR
Leo-Besancon 124ca12
tests: Add computed indices E2E test
Leo-Besancon 5456c4b
tests(parser): Add computed indices parser tests
Leo-Besancon 00df247
fix(parser): fix binding type access on non constant index
Leo-Besancon b3bc934
tests(mir): add mir tests for computed indices
Leo-Besancon 52dbd20
chore: update CHANGELOG.md
Leo-Besancon 2891862
tests: Update E2E test to have correct trace
Leo-Besancon 1202dec
chore: use chained if lets
Leo-Besancon 6634e61
tests: add computed_indices_complex E2E test
Leo-Besancon f9f7689
tests: Sort winterfell tests alphabetically
Leo-Besancon d4a07ab
fix: duplicate_node for argument nodes in calls
Leo-Besancon 8af9c70
fix: Run Mir inlining until fixed point
Leo-Besancon 62f6ddd
Merge branch 'next' into allow-computed-indices-upstream
Leo-Besancon a150733
refactor: Handle range as constant, remove their propagation into MIR
Leo-Besancon 9b01f26
refactor: multiple cleanups addressing review comments
Leo-Besancon b27ec35
refactor(mir translate): remove FIXMEs and factorize code
Leo-Besancon e63ae1a
refactor(mir): Factorize code between constant propagation and unrolling
Leo-Besancon 2b0de7d
Merge branch 'next' into allow-computed-indices-upstream
Leo-Besancon 85d4dc2
Merge branch 'next' into allow-computed-indices-upstream
Leo-Besancon b40cca5
fix: small fixes after merge
Leo-Besancon 88bb70c
refactor: refactor MIR's constant_propagation visit_node
Leo-Besancon 6e734dc
feat(mir): Value::get_inner_const
Soulthym 90b5bdc
fix(mir): rename handle_accessor_visit arg
Soulthym abbf525
fix(parser): make AccessType::Matrix unreachable for TraceBinding
Soulthym c8adbf5
fix(mir): improve docs and unify `expect_constant_indices` accross APIs.
Soulthym 82cd993
test(mir): adapt computed_indices_complex to use the trace as state
Soulthym 770b6b6
tests: rework computed indices test based on review, add comments
Leo-Besancon c48bfd4
chore: lint fix
Leo-Besancon a988ae5
Merge branch 'next' into allow-computed-indices-upstream
Leo-Besancon d7b557f
refactor: remove unneeded Option
Leo-Besancon 02610a4
Merge branch 'next' into allow-computed-indices-upstream
Leo-Besancon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61 changes: 61 additions & 0 deletions
61
air-script/tests/computed_indices/computed_indices_complex.air
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| def ComputedIndicesAir | ||
|
|
||
| const MDS = [ | ||
| [1, 2], | ||
| [2, 3], | ||
| [3, 4] | ||
| ]; | ||
|
|
||
| trace_columns { | ||
| main: [a[2], s[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; | ||
| } | ||
|
|
||
| # Note: | ||
| # In this test, we aim to test that computed indices work well even if: | ||
| # - The value of the index can only be known late during the compilation process (during MIR's constant propagation) | ||
| # - The | ||
| integrity_constraints { | ||
|
|
||
| # vec_1 is a list_comprehension that depends on the state | ||
| # vec_1 = [ | ||
| # 1 * s[0] + 2 * s[1], | ||
| # 2 * s[0] + 3 * s[1], | ||
| # 3 * s[0] + 4 * s[1] | ||
| # ]; | ||
| let vec_1 = apply_mds(s); | ||
|
|
||
| let state_2 = [2, 0]; | ||
| # vec_2 is a list_comprehension that will not get constant-folded early, but will produce constant values | ||
| let vec_2 = apply_mds(state_2); | ||
|
|
||
| # x will get the value 2 * 2 - 4 = 0 | ||
| let x = double(2) - vec_2[1]; | ||
|
|
||
| # y will get the value vec_2[0] = 2 | ||
| let y = vec_2[x]; | ||
|
|
||
| # z will then be vec_1[2] = 3 * s[0] + 4 * s[1] | ||
| let z = vec_1[y]; | ||
|
|
||
| # we enforce 3 * s[0] + 4 * s[1] = 0 | ||
| enf z = 0; | ||
| } | ||
|
|
||
| # We use apply_mds function to produce a list comprehension that will not get constant-folded during AST | ||
| fn apply_mds(state: felt[2]) -> felt[3] { | ||
| return [sum([s * m for (s, m) in (state, mds_row)]) for mds_row in MDS]; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is the part that was not possible before, right?
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.
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?
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.
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.
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.
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
In the list_comprehension_nested test, it is indeed applied to the state as you suggested below, so we can check the rust code.
What is new is to be able to index into the
vec_1andvec_2list comprehensions with indices whose can only be computed late into the compilation process (for instance, because they rely on list comprehensions)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.