Skip to content

Allow more expressions to be used in array sizes#440

Merged
jfecher merged 7 commits intomasterfrom
jf/array-sugar
Nov 9, 2022
Merged

Allow more expressions to be used in array sizes#440
jfecher merged 7 commits intomasterfrom
jf/array-sugar

Conversation

@jfecher
Copy link
Copy Markdown
Contributor

@jfecher jfecher commented Nov 4, 2022

Related issue(s)

Resolves #415

Description

Summary of changes

Allows use of global variables and simple arithmetic expressions in addition to integer literals within array-size expressions.

Previously an array with repeated-elements like the following: [0; 42] required an integer literal like 42 for the
count of repeated elements. This PR expands this a bit such that it may include global integer variables, and simple
operations on them: +, -, *, /, %, &, |, ^, and !.

This is done by adding a mini-evaluator during name resolution which is not ideal. Ideally, we could allow any comptime Field expression by delaying this until ssa and relying on the inlining done in that pass to remove all function calls, comptime variable references, and fold all arithmetic. For now though, this PR brings us most of the way there.

Test additions / changes

Adds a quick addition to the globals test.

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

@jfecher jfecher requested review from guipublic and vezenovm November 4, 2022 17:25
@kevaundray
Copy link
Copy Markdown
Contributor

So in the future, ideally, we will need to refactor this code out and move it to the ssa?

@jfecher
Copy link
Copy Markdown
Contributor Author

jfecher commented Nov 4, 2022

If we want to allow for all comptime expressions, yes. Either that or it can be done with terra's scheme of a wrapper language I mentioned before

@kevaundray
Copy link
Copy Markdown
Contributor

Alright, if we merge , can you open up another issue to support all comptime expressions, referencing this PR?

@jfecher
Copy link
Copy Markdown
Contributor Author

jfecher commented Nov 7, 2022

Made #445

@guipublic
Copy link
Copy Markdown
Contributor

SSA is already folding arithmetic expressions so there is not need to add the mini-evaluator to the SSA pass.
For now, we need to know the array size because of the monomorphisation which is done before SSA.
If we want in future to use the SSA folding for this, that mean we need somehow to support arrays with unknown size until we get there. I wonder how difficult that would be.

@jfecher
Copy link
Copy Markdown
Contributor Author

jfecher commented Nov 7, 2022

Agreed @guipublic. Those difficulties were ultimately why I ended up with this less flexible approach instead.

Comment thread crates/noirc_frontend/src/hir/resolution/resolver.rs
Comment thread crates/noirc_frontend/src/hir/resolution/resolver.rs
@jfecher jfecher requested a review from vezenovm November 9, 2022 14:49
@jfecher jfecher merged commit 62e493b into master Nov 9, 2022
@jfecher jfecher deleted the jf/array-sugar branch November 9, 2022 15:56
Copy link
Copy Markdown
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

I think the mini-evaluator is simple enough so that it won't have nor cause any big issue, yet it already brings a lot of flexibility for the user

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.

Support usage of consts in array construction

4 participants