Skip to content

feat: simply cfg immediately before flattening#9275

Merged
TomAFrench merged 5 commits intomv/array-get-side-effectsfrom
tf/more-simpler
Jul 22, 2025
Merged

feat: simply cfg immediately before flattening#9275
TomAFrench merged 5 commits intomv/array-get-side-effectsfrom
tf/more-simpler

Conversation

@TomAFrench
Copy link
Member

Description

Problem*

Resolves

Summary*

This PR adds another simplification pass immediately before cfg flattening. I've also added a pre-check to flattening which asserts that there are no constant condition jmpifs.

Additional Context

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.

@TomAFrench TomAFrench requested a review from vezenovm July 22, 2025 14:12
@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

Changes to Brillig bytecode sizes

Generated at commit: 06d129639ee3c4192141d44a4a7216ebd715de00, compared to commit: efbe355d7a8a6593c672734e6e7e85522c96fa7b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
slice_regex_inliner_zero -23 ✅ -1.42%
slice_regex_inliner_max -168 ✅ -8.43%

Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap_inliner_max 17,573 (+95) +0.54%
slice_regex_inliner_zero 1,600 (-23) -1.42%
slice_regex_inliner_max 1,824 (-168) -8.43%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 06d129639ee3c4192141d44a4a7216ebd715de00, compared to commit: efbe355d7a8a6593c672734e6e7e85522c96fa7b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
hashmap_inliner_max +303 ❌ +0.58%
slice_regex_inliner_max -98 ✅ -3.40%

Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap_inliner_max 52,394 (+303) +0.58%
slice_regex_inliner_zero 3,859 (-22) -0.57%
slice_regex_inliner_max 2,782 (-98) -3.40%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

Changes to circuit sizes

Generated at commit: 06d129639ee3c4192141d44a4a7216ebd715de00, compared to commit: efbe355d7a8a6593c672734e6e7e85522c96fa7b

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 -1,482 ✅ -5.25% -4,266 ✅ -4.93%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 26,724 (-1,482) -5.25% 82,251 (-4,266) -4.93%

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

I didn't see much benefit for the no_predicates_numeric_generic_poseidon test you had mentioned in #9232 and it looks like the reports confirm that. However, I am good with this change as it still provides benefits and simplify CFG is a more lightweight pass.

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 Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 476bb4b Previous: 3c18e33 Ratio
sha512-100-bytes 0.102 s 0.056 s 1.82

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

CC: @TomAFrench

@TomAFrench
Copy link
Member Author

I'm not seeing a change in no_predicates_numeric_generic_poseidon so it seems like this is a result of using no_predicates.

TomAFrench and others added 2 commits July 22, 2025 15:21
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
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: 476bb4b Previous: 3c18e33 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 132 s 5 s 26.40
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 152 s 42 s 3.62
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 180 s 3 s 60
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 266 s 1 s 266
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib 35 s 2 s 17.50
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 718 s 3 s 239.33
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 126 s 1 s 126
test_report_zkpassport_noir-ecdsa_ 118 s 3 s 39.33

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

CC: @TomAFrench

@TomAFrench
Copy link
Member Author

Ok, the issue is that we have control flow in the no_predicates function which depends on the arguments to the function. We perform flattening in this function with the understanding the both branches are reachable, however once we inline this into the calling function we end up discovering that we'll never go down one of these branches.

This then results in us having a enable_side_effects u1 0 popping up, except now it's happening later on in the compilation pipeline.

@TomAFrench TomAFrench merged commit ccf0343 into mv/array-get-side-effects Jul 22, 2025
29 checks passed
@TomAFrench TomAFrench deleted the tf/more-simpler branch July 22, 2025 15:02
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.

2 participants