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

fix(brillig): Brilig entry point analysis and function specialization through duplication #7277

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

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 4, 2025

Description

Problem*

Resolves #7306.

The description in this PR repeats a lot of the comments and tests from the new SSA pass, but I have copied things here for visibility.

Take this program:

global ONE: Field = 1;
global TWO: Field = 2;
global THREE: Field = 3;
fn main(x: Field, y: pub Field) {
    /// Safety: testing context
    unsafe {
        entry_point_one(x, y);
        entry_point_two(x, y);
    }
}
unconstrained fn entry_point_one(x: Field, y: Field) {
    let z = ONE + x + y;
    assert(z == 2);
    inner_func(x, y);
}
unconstrained fn entry_point_two(x: Field, y: Field) {
    let z = TWO + x + y;
    assert(z == 3);
    inner_func(x, y);
}
unconstrained fn inner_func(x: Field, y: Field) {
    let z = THREE + x + y;
    assert(z == 4);
}

We have this SSA before Brillig gen:

g0 = Field 1
g1 = Field 2
g2 = Field 3

acir(inline) predicate_pure fn main f0 {
  b0(v3: Field, v4: Field):
    call f1(v3, v4)
    call f2(v3, v4)
    return
}
brillig(inline) predicate_pure fn entry_point_one f1 {
  b0(v3: Field, v4: Field):
    v5 = add Field 1, v3
    v6 = add v5, v4
    constrain v6 == Field 2
    call f3(v3, v4)
    return
}
brillig(inline) predicate_pure fn entry_point_two f2 {
  b0(v3: Field, v4: Field):
    v5 = add Field 2, v3
    v6 = add v5, v4
    constrain v6 == Field 3
    call f3(v3, v4)
    return
}
brillig(inline) predicate_pure fn inner_func f3 {
  b0(v3: Field, v4: Field):
    v5 = add Field 3, v3
    v6 = add v5, v4
    constrain v6 == Field 4
    return
}

For each entry point we will generate the follows artifacts for globals initialization:

GlobalInit(Id(1)):
  CONST M32835 = 1
  CONST M32836 = 2
  CONST M32837 = 3
  RETURN
GlobalInit(Id(2)):
  CONST M32835 = 2
  CONST M32836 = 3
  RETURN

It is then not clear to inner_func which map of SSA value id -> Brillig global initialization we should use.
Without the hack from #7306 we get the following bytecode for f3:

Function(Id(3), Some(Id(0))):
  S3 = M32836 + S1
  S1 = S3 + S2
  CONST S2 = 4
  S3 = S1 == S2
  JUMP_IF S3 TO Function(Id(3), Some(Id(0))) - 1
  CONST S4 = 0
  TRAP FreeMem[0..S4]
  RETURN

This works when f3 is called from the f2 entry point. But it will break when called from the f1 entry point. We need to specialize f3 to account for the potentially conflicting global allocation maps.

Summary*

We now specialize functions per entry point.

A few things were done in this PR:

  1. A new pass the specializes functions per entry point
  2. I run the new pass following DIE as to reduce the amount of code we may have to duplicate. However, this also required re-running DIE following this pass as the used_globals map is set during DIE. Perhaps we do not have to re-run this though as the global value IDs should not change across SSA passes. I could foresee the CFG simplification after DIE leading to some terminators that use globals being removed and needing to recompute the used globals map. Perhaps we could recompute this during CFG simplification. I feel this can be done in a followup PR though.
  3. I have been testing against aztec-packages directly and added a few simplified regression tests to our integration tests as well as some unit tests for the new SSA pass.

Additional Context

This works further re-iterates the need for a debug/release mode as laid out in #2128. This duplication can lead to code bloat. During debug mode we could potentially not perform any entry point analysis and just have the same globals across all functions if this work is shown to seriously degrade compilation. Then during release mode we can perform the full entry point analysis to optimize for runtime.

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 added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 4, 2025
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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: a850ec0 Previous: 25b989f Ratio
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 71 s 52 s 1.37

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

CC: @TomAFrench

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Changes to Brillig bytecode sizes

Generated at commit: 80d869848cfe6174f3c4b0f1a16b9dbc3393ee58, compared to commit: 0d78578981bfcc4aa021dcc0f0238548f6ff9ca0

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
fold_complex_outputs_inliner_min -21 ✅ -2.97%
brillig_nested_arrays_inliner_zero -8 ✅ -3.32%
regression_6674_2_inliner_zero -6 ✅ -3.75%
nested_arrays_from_brillig_inliner_min -8 ✅ -4.28%
nested_array_in_slice_inliner_max -127 ✅ -11.22%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_5252_inliner_max 4,453 (-3) -0.07%
slices_inliner_min 2,534 (-3) -0.12%
slice_regex_inliner_min 2,124 (-3) -0.14%
strings_inliner_min 1,135 (-3) -0.26%
conditional_regression_short_circuit_inliner_min 1,118 (-3) -0.27%
6_inliner_min 1,017 (-3) -0.29%
strings_inliner_max 949 (-3) -0.32%
strings_inliner_zero 934 (-3) -0.32%
conditional_regression_short_circuit_inliner_zero 922 (-3) -0.32%
regression_6674_3_inliner_min 864 (-3) -0.35%
6_inliner_zero 840 (-3) -0.36%
hashmap_inliner_zero 7,843 (-45) -0.57%
higher_order_functions_inliner_zero 729 (-6) -0.82%
slice_regex_inliner_max 2,298 (-21) -0.91%
struct_inputs_inliner_min 289 (-3) -1.03%
slice_regex_inliner_zero 1,686 (-18) -1.06%
higher_order_functions_inliner_max 559 (-6) -1.06%
struct_inputs_inliner_max 269 (-3) -1.10%
array_dynamic_nested_blackbox_input_inliner_min 1,196 (-14) -1.16%
struct_inputs_inliner_zero 254 (-3) -1.17%
regression_6674_2_inliner_max 203 (-3) -1.46%
array_dynamic_nested_blackbox_input_inliner_zero 881 (-14) -1.56%
array_dynamic_nested_blackbox_input_inliner_max 854 (-14) -1.61%
nested_array_dynamic_inliner_max 2,382 (-45) -1.85%
regression_6674_1_inliner_zero 157 (-3) -1.88%
wildcard_type_inliner_min 285 (-7) -2.40%
wildcard_type_inliner_max 274 (-7) -2.49%
wildcard_type_inliner_zero 274 (-7) -2.49%
brillig_nested_arrays_inliner_min 267 (-8) -2.91%
fold_complex_outputs_inliner_min 686 (-21) -2.97%
brillig_nested_arrays_inliner_zero 233 (-8) -3.32%
regression_6674_2_inliner_zero 154 (-6) -3.75%
nested_arrays_from_brillig_inliner_min 179 (-8) -4.28%
nested_array_in_slice_inliner_max 1,005 (-127) -11.22%

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 80d869848cfe6174f3c4b0f1a16b9dbc3393ee58, compared to commit: 0d78578981bfcc4aa021dcc0f0238548f6ff9ca0

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_nested_arrays_inliner_zero -14 ✅ -4.65%
wildcard_type_inliner_min -24 ✅ -5.22%
wildcard_type_inliner_max -24 ✅ -5.39%
wildcard_type_inliner_zero -24 ✅ -5.39%
nested_array_in_slice_inliner_max -105 ✅ -7.18%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_5252_inliner_max 854,218 (-3) -0.00%
slice_regex_inliner_min 8,634 (-3) -0.03%
conditional_regression_short_circuit_inliner_min 4,977 (-3) -0.06%
hashmap_inliner_zero 74,631 (-45) -0.06%
6_inliner_min 4,871 (-3) -0.06%
conditional_regression_short_circuit_inliner_zero 4,538 (-3) -0.07%
6_inliner_zero 4,458 (-3) -0.07%
slices_inliner_min 4,289 (-3) -0.07%
strings_inliner_min 2,893 (-3) -0.10%
strings_inliner_zero 2,098 (-3) -0.14%
regression_6674_3_inliner_min 2,003 (-3) -0.15%
strings_inliner_max 1,743 (-3) -0.17%
array_dynamic_nested_blackbox_input_inliner_min 4,954 (-12) -0.24%
array_dynamic_nested_blackbox_input_inliner_zero 4,500 (-12) -0.27%
array_dynamic_nested_blackbox_input_inliner_max 4,248 (-12) -0.28%
slice_regex_inliner_zero 4,041 (-18) -0.44%
higher_order_functions_inliner_zero 1,328 (-6) -0.45%
struct_inputs_inliner_min 659 (-3) -0.45%
regression_6674_1_inliner_zero 619 (-3) -0.48%
regression_6674_2_inliner_max 606 (-3) -0.49%
struct_inputs_inliner_zero 601 (-3) -0.50%
struct_inputs_inliner_max 566 (-3) -0.53%
slice_regex_inliner_max 3,232 (-21) -0.65%
higher_order_functions_inliner_max 901 (-6) -0.66%
regression_6674_2_inliner_zero 616 (-6) -0.96%
nested_array_dynamic_inliner_max 3,366 (-39) -1.15%
fold_complex_outputs_inliner_min 1,067 (-18) -1.66%
nested_arrays_from_brillig_inliner_min 215 (-7) -3.15%
brillig_nested_arrays_inliner_min 468 (-21) -4.29%
brillig_nested_arrays_inliner_zero 287 (-14) -4.65%
wildcard_type_inliner_min 436 (-24) -5.22%
wildcard_type_inliner_max 421 (-24) -5.39%
wildcard_type_inliner_zero 421 (-24) -5.39%
nested_array_in_slice_inliner_max 1,357 (-105) -7.18%

@vezenovm vezenovm changed the title fix(brillig): Duplicate Brillig entry points called by Brillig entry points fix(brillig): Brilig entry point analysis and function specialization through duplication Feb 7, 2025
@vezenovm vezenovm marked this pull request as ready for review February 7, 2025 13:12
@vezenovm vezenovm requested a review from a team February 7, 2025 13:18
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.

Sync temp fix overriding global reachability analysis
1 participant