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: SSA globals in monomorphization and SSA gen #6985

Merged
merged 27 commits into from
Jan 10, 2025
Merged

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jan 8, 2025

Description

Problem*

Working towards #6801

Summary*

This PR does the initial work to move towards handling globals in SSA. Actually utilizing globals in Brillig gen will come in a follow-up PR. I wanted to split out this work for easier reviewing as all the globals work together will be a good amount of code.

I wanted to add globals to SSA without any major changes to our existing data structures and or having to thread lifetimes throughout our structures. In follow-up work we most likely will want to further abstract some our concepts such as the DataFlowGraph or pass a global context into all SSA functions. For now, I wanted to focus on minimal changes without causing regressions.

The main changes:

  1. During monomorphization we now keep track of globals and have a new Definition::Global. If we have a new global we monomorphize the global's expression but then wrap it in an ident with a Definition::Global. For closures we keep the old strategy of inlining the global during monomorphization.
  2. The SharedContext now contains a GlobalsContext
  3. The GlobalsContext re-uses the DataFlowGraph to build globals. The newly built globals are stored in a globals: BTreeMap<monomorphization::ast::GlobalId, Values> map to be accessed by the FunctionContext.
  4. Before performing any code gen for a function we full up the value IDs that are reserved for globals.
  5. A global is represented in a function's DFG simply with a Value::Global. When we see Value::Global it indicates we have a reserved value id and that we need to access some separate global context for the actual value. Currently, the only pass that makes use of this is inlining. We are currently still inlining all globals so that the globals work can be split up for easier reviewing.

Example SSA for the new global_var_regression_simple test (commenting out assert(!acc.lt(x)) for a simpler SSA):

SSA output with globals
After Initial SSA:
g0 = Field 1
g1 = make_array [Field 1, Field 1] : [Field; 2]
g2 = Field 0
g3 = make_array [Field 0, Field 0] : [Field; 2]
g4 = make_array [v1, v3] : [[Field; 2]; 2]

brillig(inline) fn main f0 {
  b0(v5: Field, v6: Field):
    v9 = allocate -> &mut Field
    store Field 0 at v9
    jmp b1(u32 0)
  b1(v7: u32):
    v13 = lt v7, u32 2
    jmpif v13 then: b3, else: b2
  b2():
    v14 = eq v5, v6
    v15 = not v14
    constrain v14 == u1 0
    call f1(v5, v6)
    return
  b3():
    jmp b4(u32 0)
  b4(v8: u32):
    v18 = lt v8, u32 2
    jmpif v18 then: b6, else: b5
  b5():
    v20 = add v7, u32 1
    jmp b1(v20)
  b6():
    v21 = load v9 -> Field
    v22 = array_get g4, index v7 -> [Field; 2]
    inc_rc v22
    v23 = array_get v22, index v8 -> Field
    v24 = add v21, v23
    store v24 at v9
    v25 = add v8, u32 1
    jmp b4(v25)
}
brillig(inline) fn dummy_again f1 {
  b0(v5: Field, v6: Field):
    v9 = allocate -> &mut Field
    store Field 0 at v9
    jmp b1(u32 0)
  b1(v7: u32):
    v13 = lt v7, u32 2
    jmpif v13 then: b3, else: b2
  b2():
    v14 = eq v5, v6
    v15 = not v14
    constrain v14 == u1 0
    return
  b3():
    jmp b4(u32 0)
  b4(v8: u32):
    v17 = lt v8, u32 2
    jmpif v17 then: b6, else: b5
  b5():
    v19 = add v7, u32 1
    jmp b1(v19)
  b6():
    v20 = load v9 -> Field
    v21 = array_get g4, index v7 -> [Field; 2]
    inc_rc v21
    v22 = array_get v21, index v8 -> Field
    v23 = add v20, v22
    store v23 at v9
    v24 = add v8, u32 1
    jmp b4(v24)
}

Additional Context

Actual implementation of globals and handling them as we do existing constants will come in followups. It may be helpful to wait for the PR with the Brillig gen implementation to see how globals will actually be used.

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.

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Changes to Brillig bytecode sizes

Generated at commit: 2c3f7b9baeeb52d9903243aff5a25c92b5d1b24f, compared to commit: e71fcdfebd92c349d4b2f52d87ead2b18edfcd4a

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bigint -12 ✅ -0.55%

Full diff report 👇
Program Brillig opcodes (+/-) %
bigint 2,153 (-12) -0.55%

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Execution Report

Program Execution Time
sha256_regression 0.055s
regression_4709 0.001s
ram_blowup_regression 0.576s
rollup-root 0.106s
rollup-block-merge 0.106s
rollup-base-public 1.246s
rollup-base-private 0.474s
private-kernel-tail 0.021s
private-kernel-reset 0.354s
private-kernel-inner 0.077s

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Compilation Report

Program Compilation Time
sha256_regression 1.316s
regression_4709 0.787s
ram_blowup_regression 18.220s
rollup-root 3.440s
rollup-block-merge 3.558s
rollup-base-public 39.960s
rollup-base-private 13.400s
private-kernel-tail 1.025s
private-kernel-reset 8.600s
private-kernel-inner 2.046s

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Compilation Memory Report

Program Peak Memory
keccak256 78.13M
workspace 123.89M
regression_4709 424.09M
ram_blowup_regression 1.58G
rollup-base-public 4.85G
rollup-base-private 1.26G
private-kernel-tail 207.14M
private-kernel-reset 730.52M
private-kernel-inner 294.37M

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Execution Memory Report

Program Peak Memory
keccak256 74.67M
workspace 123.79M
regression_4709 316.00M
ram_blowup_regression 512.42M
rollup-base-public 482.47M
rollup-base-private 328.60M
private-kernel-tail 180.95M
private-kernel-reset 249.60M
private-kernel-inner 209.79M

@TomAFrench
Copy link
Member

I'm sure you're thinking about this but looks like we don't have global arrays yet which I expect would be the biggest contributor here.

@vezenovm
Copy link
Contributor Author

vezenovm commented Jan 8, 2025

I'm sure you're thinking about this but looks like we don't have global arrays yet which I expect would be the biggest contributor here.

Are you looking at just Value::Global? This is simply a value to indicate we should look in a separate global context for the correct value. We do have global arrays as part of this work. GlobalsContext shows all the values we support building. I have yet to write out the description which will provide some more info.

@TomAFrench
Copy link
Member

Ah gotcha, I'm jumping the gun in that case.

@vezenovm vezenovm marked this pull request as ready for review January 8, 2025 18:53
Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Mostly minor comments but the main item is avoiding duplication of codegen functions

compiler/noirc_evaluator/src/ssa/ir/printer.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs Outdated Show resolved Hide resolved
compiler/noirc_frontend/src/hir/comptime/value.rs Outdated Show resolved Hide resolved
compiler/noirc_frontend/src/monomorphization/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_frontend/src/monomorphization/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/ir/value.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs Outdated Show resolved Hide resolved
@vezenovm vezenovm requested a review from jfecher January 9, 2025 21:17
@vezenovm vezenovm requested a review from TomAFrench January 10, 2025 17:48
@TomAFrench TomAFrench enabled auto-merge January 10, 2025 18:36
@TomAFrench
Copy link
Member

@jfecher

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the ssa-gen change

@TomAFrench TomAFrench added this pull request to the merge queue Jan 10, 2025
Merged via the queue into master with commit db28cb9 Jan 10, 2025
88 checks passed
@TomAFrench TomAFrench deleted the mv/ssa-globals-1 branch January 10, 2025 20:24
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 11, 2025
…ang/noir#6985)

feat!: disallow calling unconstrained functions outside of `unsafe` blocks and passing unconstrained functions in place of constrained functions (noir-lang/noir#6938)
chore: simplify a couple of enum variants (noir-lang/noir#7025)
chore: disallow inserting ACIR-only instructions into brillig functions (noir-lang/noir#7017)
feat: unchecked math operations in SSA (noir-lang/noir#7011)
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 11, 2025
feat!: disallow calling unconstrained functions outside of `unsafe` blocks and passing unconstrained functions in place of constrained functions (noir-lang/noir#6938)
chore: simplify a couple of enum variants (noir-lang/noir#7025)
chore: disallow inserting ACIR-only instructions into brillig functions (noir-lang/noir#7017)
feat: unchecked math operations in SSA (noir-lang/noir#7011)
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
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.

4 participants