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

feat(brillig): SSA globals code gen #7021

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jan 10, 2025

Description

Problem*

Resolves #6801

Summary*

Implements globals from SSA in Brillig gen. We create a new memory space GlobalSpace at compile time. Globals are declared in the global space and the linker is then used to connect these with our SSA functions. When converting SSA values we check whether we have a global and if so, look for the appropriate register that keys into the appropriate slot in the global space.

Additional Context

Some slight regressions are expected. Some reasons for this are laid out here #7021 (comment).

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

vezenovm and others added 30 commits January 8, 2025 02:46
Base automatically changed from mv/ssa-globals-1 to master January 10, 2025 20:24
@TomAFrench TomAFrench marked this pull request as ready for review January 15, 2025 18:14
@TomAFrench TomAFrench marked this pull request as draft January 15, 2025 18:14
@vezenovm
Copy link
Contributor Author

The remaining regressions look to be due to dead globals not being removed. Will have a PR shortly.

@vezenovm vezenovm mentioned this pull request Jan 15, 2025
5 tasks
@vezenovm
Copy link
Contributor Author

vezenovm commented Jan 15, 2025

The remaining regressions look to be due to dead globals not being removed. Will have a PR shortly.

I tried building off master originally but it was a lot easier to test, building off this branch. The work can be seen here in #7081.

diamond_deps_0_inliner_zero +2 ❌ +7.69%
modules_inliner_zero +2 ❌ +7.69%
modules_more_inliner_zero +2 ❌ +7.69%
no_predicates_basic_inliner_zero +2 ❌ +7.69%
traits_in_crates_1_inliner_zero +2 ❌ +7.69%
traits_in_crates_2_inliner_zero +2 ❌ +7.69%
fold_basic_inliner_zero +2 ❌ +7.69%

The globals DIE in #7081 is also not going to get rid of these extra instructions we see here. add_globals_init_instruction() makes an external call to our globals initialization. Without any globals to declare we will just execute a return instruction. Thus, we see an overhead of 2 instructions, a call and a return. We can get rid of this overhead, but it would require some larger changes that felt not worth holding up this PR on its own and could come in a follow-up. In general once this PR is in we will probably want to do a refactor that unifies how we track globals.

The affect on programs that make more meaningful usage of globals across functions can still be seen.
Some example bytecode size reductions:

global_var_regression_simple_inliner_min | 240 (-21) | -8.05%
bigint_inliner_min | 2,104 (-360) | -14.61%
bigint_inliner_zero | 1,906 (-332) | -14.83%

@vezenovm vezenovm marked this pull request as ready for review January 15, 2025 21:05
@vezenovm vezenovm requested review from sirasistant and a team January 16, 2025 00:42
@vezenovm
Copy link
Contributor Author

vezenovm commented Jan 16, 2025

I look to be getting failures post AztecProtocol/aztec-packages#10605. Working on resolving it now.

EDIT: Ah it is all Global space too deep errors. I had to bump it to 16384.

tooling/nargo/src/errors.rs Outdated Show resolved Hide resolved
@TomAFrench
Copy link
Member

I look to be getting failures post AztecProtocol/aztec-packages#10605.

I assume that this is from the roots global. This isn't used in brillig code currently so I imagine this would be solved by the die pass PR

That said, as we know all of the globals at compile time we should be able to expand the space allocated as necessary. If so can you create a follow up issue?

@vezenovm
Copy link
Contributor Author

vezenovm commented Jan 16, 2025

That said, as we know all of the globals at compile time we should be able to expand the space allocated as necessary. If so can you create a follow up issue?

Yeah I'll make an issue for the max global space to be determined by the program rather than a hardcoded value.

@@ -32,28 +32,38 @@ use super::brillig_fn::FunctionContext;
use super::constant_allocation::InstructionLocation;

/// Generate the compilation artifacts for compiling a function into brillig bytecode.
pub(crate) struct BrilligBlock<'block> {
pub(crate) function_context: &'block mut FunctionContext,
pub(crate) struct BrilligBlock<'block, 'global, Registers: RegisterAllocator> {
Copy link
Member

@TomAFrench TomAFrench Jan 16, 2025

Choose a reason for hiding this comment

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

Do we need two lifetimes here? I'd expect that the globals would outlive 'block anyway so we can just reuse that lifetime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, not sure why I put them on the FunctionContext, good point. I think with how I have it now with the globals inside of FunctionContext I cannot avoid the second lifetime, but I will move it out of there and put it directly on BrilligBlock.

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.

SSA: Global variables
2 participants