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

mem2reg on array arguments to main #1411

Closed
Tracked by #1376
joss-aztec opened this issue May 26, 2023 · 2 comments
Closed
Tracked by #1376

mem2reg on array arguments to main #1411

joss-aztec opened this issue May 26, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request refactor ssa

Comments

@joss-aztec
Copy link
Contributor

Problem

Currently mem2reg is used to promote const-indexed array store/loads to registers, to the end that such array allocations can be removed. However, arrays appearing in parameters to main are treated as a special case (regardless of whether they are dynamic or const indexed). As a result we need to maintain extra code in ACIR gen for converting these const-index array parameters to main into ACIR.

The main argument for this special treatment is that were it the case that an array param to main were indexed dynamically, the effects of the mem2reg pass would be complex to reason through.

Happy Case

Once dynamic arrays have been implemented for the SSA refactor, it may be worth returning to review whether or not it simplifies matters to remove this special edge-case.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@guipublic
Copy link
Contributor

As far as I can tell:

  • with the dynamic array implementation, handling of const-index array operations are required in acir-gen anyways, so we do not need to maintain extra code for array arguments of main.
  • I did not see any special case dedicated to main array arguments in mem2reg. As parameters to the first block, they are assigned an unknown value because the first block has no predecessor.

@jfecher
Copy link
Contributor

jfecher commented Oct 3, 2023

Closing this as outdated. We used to use mutable arrays internally which required the mem2reg pass to track gets/sets to each index. We've since moved to immutable arrays so that they may be simplified anywhere and mem2reg is no longer concerned with them (apart from tracking arrays which store references).

@jfecher jfecher closed this as completed Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor ssa
Projects
Archived in project
Development

No branches or pull requests

4 participants