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: add pass which inlines constant arguments into brillig functions #7559

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

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Feb 28, 2025

Description

Problem

Resolves #6812

Summary

Pending:

  • Check what happens if the brillig call is recursive
  • Determine if the >= 30% heuristic is good
  • Determine if the existing optimizations to the new function are good or we need more/others (I copied those from the preprocess pass)
  • Add more tests
  • Code cleanup

Additional Context

Also includes a commit to get better feedback on SSA test failures, and fixing some indentation in tests.

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 Feb 28, 2025

Changes to circuit sizes

Generated at commit: 304d899f9e8b73a7ccb4956478a3cb80d263a04e, compared to commit: 5b725bb77b852f812073be2b7cb51130505d22b6

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
reference_counts +5 ❌ +62.50% +5 ❌ +31.25%
global_var_regression_simple +7 ❌ +10.61% +16 ❌ +0.55%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
reference_counts 13 (+5) +62.50% 21 (+5) +31.25%
global_var_regression_simple 73 (+7) +10.61% 2,930 (+16) +0.55%

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
rollup-block-root 7870 MB 1420 MB 5.54

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@TomAFrench TomAFrench added the bench-show Display benchmark results on PR label Feb 28, 2025
@asterite
Copy link
Collaborator Author

I don't understand why these regressions are happening (and less why the "stack too deep" error happens). I'm looking at the final SSA before and after this PR, and the new SSA is shorter. In the case of program "6" the main function is exactly the same, except that some calls have less arguments because they have been inlined. nargo info tells me it's more ACIR opcodes with this PR, but it's less brillig opcodes, but that's somehow not reflected in the reports. With --profile-execution the executed brillig opcodes are the same, though.

@asterite
Copy link
Collaborator Author

I'm also trying it with this program:

use bignum::BigNum;
use bignum::BigNumTrait;
use bignum::fields::U256::U256Params;

// Define (compile-time) BigNum type
// number of limbs, number of bits of modulus, parameter set
type U256 = BigNum<3, 257, U256Params>;

fn main(x: [Field; 3], y: [Field; 3]) -> pub [Field; 3] {
    let one = U256::from_slice(x);
    let two = U256::from_slice(y);
    // (one + two).limbs
    (one + two).limbs
}

This is the output of nargo info in master:

+---------+------------------+----------------------+--------------+-----------------+
| Package | Function         | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+---------+------------------+----------------------+--------------+-----------------+
| one     | main             | Bounded { width: 4 } | 17           | 783             |
+---------+------------------+----------------------+--------------+-----------------+
| one     | __add_with_flags | N/A                  | N/A          | 783             |
+---------+------------------+----------------------+--------------+-----------------+

(with --profile-execution)

+---------+----------+----------------------+--------------+-----------------+
| Package | Function | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+---------+----------+----------------------+--------------+-----------------+
| one     | main     | Bounded { width: 4 } | 0            | 1617            |
+---------+----------+----------------------+--------------+-----------------+
| one     | main     | N/A                  | N/A          | 1617            |
+---------+----------+----------------------+--------------+-----------------+

And this is the output with this PR:

+---------+------------------+----------------------+--------------+-----------------+
| Package | Function         | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+---------+------------------+----------------------+--------------+-----------------+
| one     | main             | Bounded { width: 4 } | 17           | 719             |
+---------+------------------+----------------------+--------------+-----------------+
| one     | __add_with_flags | N/A                  | N/A          | 719             |
+---------+------------------+----------------------+--------------+-----------------+

(with --profile-execution)

+---------+----------+----------------------+--------------+-----------------+
| Package | Function | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+---------+----------+----------------------+--------------+-----------------+
| one     | main     | Bounded { width: 4 } | 0            | 1617            |
+---------+----------+----------------------+--------------+-----------------+
| one     | main     | N/A                  | N/A          | 1617            |
+---------+----------+----------------------+--------------+-----------------+

So it seems the number of brillig opcodes decreases, but the same number of opcodes is executed.

Maybe we are missing some optimizations done on the functions where constants have been inlined (for example there's no constant folding happening there, though I tried it and it didn't change anything...)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Compilation Time

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
regression_4709 0.685 s 0.736 s 0.93
ram_blowup_regression 15 s 14.9 s 1.01
global_var_regression_entry_points 0.484 s 0.505 s 0.96
private-kernel-inner 2.378 s 2.242 s 1.06
private-kernel-reset 6.648 s 6.878 s 0.97
private-kernel-tail 1.202 s 1.112 s 1.08
rollup-base-private 15.12 s 15.42 s 0.98
rollup-base-public 11.38 s 11.36 s 1.00
rollup-block-root-empty 0.942 s 0.942 s 1
rollup-merge 0.906 s 0.922 s 0.98
rollup-root 1.528 s 1.482 s 1.03

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Execution Time

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
private-kernel-inner 0.07 s 0.07 s 1
private-kernel-reset 0.29 s 0.291 s 1.00
private-kernel-tail 0.027 s 0.027 s 1
rollup-base-private 0.751 s 0.753 s 1.00
rollup-base-public 0.502 s 0.506 s 0.99
rollup-merge 0.006 s 0.007 s 0.86
rollup-root 0.025 s 0.026 s 0.96

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Test Suite Duration

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
AztecProtocol_aztec-packages_noir-projects_aztec-nr 41 s 42 s 0.98
AztecProtocol_aztec-packages_noir-projects_noir-contracts 76 s 75 s 1.01
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 174 s 168 s 1.04
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib 10 s 10 s 1
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 57 s 53 s 1.08
noir-lang_noir-bignum_ 73 s 73 s 1
noir-lang_noir_bigcurve_ 227 s 202 s 1.12
noir-lang_noir_json_parser_ 8 s 9 s 0.89
noir-lang_sha512_ 24 s 24 s 1

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Compilation Memory

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
private-kernel-inner 299.98 MB 299.98 MB 1
private-kernel-reset 611.14 MB 609.91 MB 1.00
private-kernel-tail 226.63 MB 226.59 MB 1.00
rollup-base-private 1250 MB 1250 MB 1
rollup-base-public 1330 MB 1330 MB 1
rollup-block-root-empty 303.45 MB 303.46 MB 1.00
rollup-block-root-single-tx 7860 MB 7860 MB 1
rollup-block-root 7870 MB 7870 MB 1
rollup-merge 301.86 MB 301.86 MB 1
rollup-root 349.43 MB 349.38 MB 1.00

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Execution Memory

Benchmark suite Current: 7d92c11 Previous: 5b725bb Ratio
private-kernel-inner 239.77 MB 239.83 MB 1.00
private-kernel-reset 274.21 MB 274.22 MB 1.00
private-kernel-tail 213.38 MB 213.45 MB 1.00
rollup-base-private 507.71 MB 507.62 MB 1.00
rollup-base-public 416.49 MB 416.42 MB 1.00
rollup-block-root 7870 MB 1420 MB 5.54
rollup-merge 290.42 MB 290.44 MB 1.00
rollup-root 296.87 MB 296.91 MB 1.00

This comment was automatically generated by workflow using github-action-benchmark.

@vezenovm
Copy link
Contributor

vezenovm commented Mar 3, 2025

(and less why the "stack too deep" error happens)

The rollup block root programs utilizes massive array constants. I thought we had moved all of them to globals, looks like we still pass these globals: https://github.com/AztecProtocol/aztec-packages/blob/f72ccc28641c40dc30ff63929842cb87c3c2a6d4/noir-projects/noir-protocol-circuits/crates/blob/src/blob.nr#L156

We should be able to move to using the ROOTS global being passed as an unconstrained_roots param now that globals are stable (noted here AztecProtocol/aztec-packages#10605 (comment) as well).

However, we should still account for massive constants that may overflow the stack through normal Brillig compilation. In globals compilation, the memory space available to the stack is adjusted at compile-time to account for user specified constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bench-show Display benchmark results on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pass which inlines constant arguments into brillig functions
3 participants