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

SSA Refactor: Remove tech debt when moving to the SSA refactor #1850

Closed
vezenovm opened this issue Jul 3, 2023 · 12 comments
Closed

SSA Refactor: Remove tech debt when moving to the SSA refactor #1850

vezenovm opened this issue Jul 3, 2023 · 12 comments
Assignees
Labels
enhancement New feature or request refactor ssa

Comments

@vezenovm
Copy link
Contributor

vezenovm commented Jul 3, 2023

Problem

  1. PR feat(ssa_refactor)!: Add Slices #1728 implements syntax for slices into Noir. However, as we are moving to the new SSA pass, the PR only implements slices in the new pass while breaking changes were made to the frontend in order to get slices fully working. This means that the experimental_ssa flag is now needed not only to determine which SSA to generate but also during compilation.

  2. There are features aside slices that touch the frontend but are only implemented using the new SSA. For example after the refactor in PR feat: Refactor Logging to use Brillig foreign calls #1917 we have two different implementations of println. One that uses the old refactor and one that uses Brillig foreign calls. The experimental_ssa flag is used again here to determine whether we should use one println or the other.

  3. We have unneeded transformations like the array of structs to struct of arrays conversion during monomorphization that can be removed once the old ssa code is removed. Removing this would lead to a more intuitive serialization order when going back and forth between Noir and TypeScript.

Happy Case

The enable_slices flag inside of the NodeInterner should be fully removed and all of its conditional checks within the frontend. The array of structs to struct of arrays conversion should also be removed.

Alternatives Considered

N/A

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@vezenovm vezenovm added enhancement New feature or request ssa labels Jul 3, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 3, 2023
@jfecher
Copy link
Contributor

jfecher commented Jul 10, 2023

We should also remove the array of structs -> structs of arrays conversion that happens in monomorphisation. Once the refactor is the default, it is unneeded complexity and it makes serialization order of arguments between Noir and TypeScript less intuitive. From user feedback:

Noir serializes arrays of structs packing the members together instead of the struct together, so if calling another function, for example with an array of points, instead of
PrivateCallStackItem::call(... [points[0].x, points[0].y, points[1].x, points[1].y...)
You have to do
PrivateCallStackItem::call(... [points[0].x, points[1].x, points[2].x, points[0].y...)
feels unnatural

@jfecher jfecher changed the title SSA Refactor: Remove enable_slices tech debt when moving to the SSA refactor SSA Refactor: Remove tech debt when moving to the SSA refactor Jul 10, 2023
@kevaundray
Copy link
Contributor

We can also enable 2d arrays once the old SSA is removed

@kevaundray
Copy link
Contributor

We should re-evaluate #562 when closing this too

@jfecher
Copy link
Contributor

jfecher commented Jul 17, 2023

@kevaundray, #562 should be unrelated since that is a type system bug

@kevaundray
Copy link
Contributor

@kevaundray, #562 should be unrelated since that is a type system bug

When we make this the default, we would also be merging in the breaking change which would force users to use [T;N] instead of [T], so I think that would issue may be fixed

@kevaundray
Copy link
Contributor

This issue should also be reviewed once this is closed, since this can only be closed after dynamic indices: #793

@jfecher
Copy link
Contributor

jfecher commented Jul 20, 2023

@kevaundray
Copy link
Contributor

Also adding: #1830

@kevaundray
Copy link
Contributor

Adding #1925

@kevaundray
Copy link
Contributor

This was mentioned here: #1842

@TomAFrench
Copy link
Member

I've closed the linked issues which are fixed by just using the new SSA.

@jfecher
Copy link
Contributor

jfecher commented Jul 31, 2023

All bugs and changes listed here have been completed.

@jfecher jfecher closed this as completed Jul 31, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 31, 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