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

Avoid code gen of unnecessary Brillig array offsetting #7403

Closed
vezenovm opened this issue Feb 14, 2025 · 0 comments · Fixed by #7522
Closed

Avoid code gen of unnecessary Brillig array offsetting #7403

vezenovm opened this issue Feb 14, 2025 · 0 comments · Fixed by #7522
Labels
brillig Unconstrained functions / brillig IR enhancement New feature or request

Comments

@vezenovm
Copy link
Contributor

vezenovm commented Feb 14, 2025

Problem

There are various places in Brillig gen where we seem to be unnecessarily having to increment the array or vector items pointer.

When we have an array get we need to generate the pointer to its items. Arrays are laid out as [RC, ...items], so every time we want to access the array items we have to increment by one

pub(crate) fn codegen_make_array_items_pointer(

A vector pointer requires incrementing by three.

So if we have an array get like such: array_get v1, index u32 1, we will first always increment the pointer specified with v1 by one, and only then will we apply the user specified offset. The extra increment is completely unnecessary instructions as we already have a constant index. We should be able to just increment by array_get v1, index u32 1 (user index) + 1 (RC offset) -> array_get v1, index u32 2 to fetch the correct value.

Happy Case

Avoid any unnecessary increments to get the items pointer to an array. The only places this is done is for array get, array set, and make array. I only really looked at array get, would have to see if it is possible to remove this extra RC offset instruction in all cases.

Possible options:

  1. Handling this in an SSA pass right before Brillig. If we see any constant array gets we should transform them by incrementing by one right away.
  2. SSA code gen different array index handling for the Brillig runtime.

There might be some extra challenges elsewhere in the compilation pipeline when changing array indexing. For example, arrays are handled the same for ACIR/Brillig in the instruction insertion simplification, so this change would lead to a divergence. It may be simpler to simply convert array indices right before Brillig gen.

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

None

Blocker Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@vezenovm vezenovm added brillig Unconstrained functions / brillig IR enhancement New feature or request labels Feb 14, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Feb 14, 2025
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brillig Unconstrained functions / brillig IR enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant