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(ssa): Accurately mark binary ops for hoisting and check Div/Mod against induction variable lower bound #7396

Merged
merged 8 commits into from
Feb 18, 2025

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 14, 2025

Description

Problem*

No issue but just a bug I found while working towards other tasks.

Summary*

We currently mark an invariant as being allowed for hoisting with the following:

        let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false)
            || matches!(instruction, Instruction::MakeArray { .. })
            || matches!(instruction, Instruction::Binary(_))
            || self.can_be_deduplicated_from_loop_bound(&instruction);

This means we are currently allowing any Binary op to be hoisting if it is an invariant. We need to remove the || matches!(instruction, Instruction::Binary(_)) and rely on instruction.can_be_deduplicated and self.can_be_deduplicated_from_loop_bound.

This is an issue because if we were to have an operation that can potentially error (such as from overflow or div by zero) that was under a predicate, we would hoist that instruction out of the loop. If that predicate turns off the error (e.g. by protecting against a div by zero in user code) a user would still hit an error.

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.

@vezenovm vezenovm requested a review from a team February 14, 2025 18:10
Copy link
Contributor

github-actions bot commented Feb 14, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 52ccaafbb99ea5dfa20ad1520d7b046eab9f75cd, compared to commit: 31cc6a1cf9ea0a02931ef60c71d4d41524f8b84c

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
loop_invariant_regression_inliner_max +109 ❌ +11.37%
loop_invariant_regression_inliner_zero +109 ❌ +11.37%
sha256_regression_inliner_max +8,652 ❌ +7.75%
sha256_var_padding_regression_inliner_max +14,112 ❌ +7.19%
sha256_var_size_regression_inliner_max +1,008 ❌ +6.46%
sha256_brillig_performance_regression_inliner_max +1,365 ❌ +6.38%
ram_blowup_regression_inliner_max +43,008 ❌ +6.31%
brillig_cow_regression_inliner_max +28,236 ❌ +6.22%

Full diff report 👇
Program Brillig opcodes (+/-) %
loop_invariant_regression_inliner_max 1,068 (+109) +11.37%
loop_invariant_regression_inliner_zero 1,068 (+109) +11.37%
sha256_regression_inliner_max 120,260 (+8,652) +7.75%
sha256_var_padding_regression_inliner_max 210,436 (+14,112) +7.19%
sha256_var_size_regression_inliner_max 16,612 (+1,008) +6.46%
sha256_brillig_performance_regression_inliner_max 22,768 (+1,365) +6.38%
ram_blowup_regression_inliner_max 724,802 (+43,008) +6.31%
brillig_cow_regression_inliner_max 481,955 (+28,236) +6.22%
loop_invariant_regression_inliner_min 1,762 (+95) +5.70%
sha256_regression_inliner_zero 163,804 (+8,652) +5.58%
sha256_regression_inliner_min 165,846 (+8,652) +5.50%
ram_blowup_regression_inliner_zero 856,057 (+43,008) +5.29%
sha256_brillig_performance_regression_inliner_zero 27,260 (+1,365) +5.27%
ram_blowup_regression_inliner_min 860,315 (+43,008) +5.26%
brillig_cow_regression_inliner_zero 567,588 (+28,236) +5.24%
brillig_cow_regression_inliner_min 571,185 (+28,236) +5.20%
sha256_brillig_performance_regression_inliner_min 27,641 (+1,365) +5.19%
sha256_var_padding_regression_inliner_zero 241,883 (+11,760) +5.11%
sha256_var_padding_regression_inliner_min 243,382 (+11,760) +5.08%
sha256_var_size_regression_inliner_zero 22,319 (+1,008) +4.73%
sha256_var_size_regression_inliner_min 23,074 (+1,008) +4.57%
ecdsa_secp256k1_inliner_max 6,588 (+210) +3.29%
array_dynamic_blackbox_input_inliner_zero 21,849 (+672) +3.17%
array_dynamic_blackbox_input_inliner_min 22,936 (+672) +3.02%
sha2_byte_inliner_max 44,718 (+1,309) +3.02%
ecdsa_secp256k1_inliner_zero 7,780 (+210) +2.77%
ecdsa_secp256k1_inliner_min 8,359 (+210) +2.58%
regression_5252_inliner_zero 1,033,907 (+25,550) +2.53%
poseidonsponge_x5_254_inliner_zero 207,376 (+5,110) +2.53%
regression_5252_inliner_min 1,058,149 (+25,550) +2.47%
poseidonsponge_x5_254_inliner_min 211,699 (+5,110) +2.47%
poseidon_bn254_hash_inliner_min 189,602 (+3,641) +1.96%
poseidon_bn254_hash_width_3_inliner_min 189,602 (+3,641) +1.96%
sha2_byte_inliner_zero 72,980 (+1,309) +1.83%
regression_4449_inliner_max 201,005 (+2,940) +1.48%
sha2_byte_inliner_min 90,082 (+1,309) +1.47%
regression_4449_inliner_zero 227,047 (+2,940) +1.31%
regression_5252_inliner_max 864,831 (+10,800) +1.26%
poseidonsponge_x5_254_inliner_max 173,648 (+2,160) +1.26%
6_inliner_max 6,899 (+84) +1.23%
conditional_regression_short_circuit_inliner_max 6,977 (+84) +1.22%
regression_4449_inliner_min 268,273 (+2,940) +1.11%
poseidon_bn254_hash_inliner_max 153,202 (+1,509) +0.99%
poseidon_bn254_hash_width_3_inliner_max 153,202 (+1,509) +0.99%
6_inliner_zero 4,442 (+42) +0.95%
conditional_regression_short_circuit_inliner_zero 4,522 (+42) +0.94%
6_inliner_min 4,855 (+42) +0.87%
conditional_regression_short_circuit_inliner_min 4,961 (+42) +0.85%
conditional_1_inliner_max 5,085 (+42) +0.83%
poseidon_bn254_hash_inliner_zero 183,948 (+1,509) +0.83%
poseidon_bn254_hash_width_3_inliner_zero 183,948 (+1,509) +0.83%
conditional_1_inliner_zero 5,511 (+42) +0.77%
conditional_1_inliner_min 5,980 (+42) +0.71%
sha256_var_witness_const_regression_inliner_max 6,369 (+42) +0.66%
sha256_inliner_max 13,522 (+84) +0.63%
sha256_var_witness_const_regression_inliner_zero 6,921 (+42) +0.61%
sha256_inliner_zero 11,310 (+63) +0.56%
sha256_var_witness_const_regression_inliner_min 7,714 (+42) +0.55%
sha256_inliner_min 12,246 (+63) +0.52%
array_dynamic_nested_blackbox_input_inliner_max 4,220 (+21) +0.50%
array_dynamic_nested_blackbox_input_inliner_zero 4,472 (+21) +0.47%
array_dynamic_nested_blackbox_input_inliner_min 4,926 (+21) +0.43%

Copy link
Contributor

github-actions bot commented Feb 14, 2025

Changes to Brillig bytecode sizes

Generated at commit: 52ccaafbb99ea5dfa20ad1520d7b046eab9f75cd, compared to commit: 31cc6a1cf9ea0a02931ef60c71d4d41524f8b84c

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
loop_invariant_regression_inliner_max +34 ❌ +29.31%
loop_invariant_regression_inliner_zero +34 ❌ +29.31%
loop_invariant_regression_inliner_min +39 ❌ +26.90%

Full diff report 👇
Program Brillig opcodes (+/-) %
loop_invariant_regression_inliner_max 150 (+34) +29.31%
loop_invariant_regression_inliner_zero 150 (+34) +29.31%
loop_invariant_regression_inliner_min 184 (+39) +26.90%
poseidon_bn254_hash_width_3_inliner_min 5,056 (+12) +0.24%
poseidon_bn254_hash_inliner_min 5,056 (+12) +0.24%
poseidonsponge_x5_254_inliner_zero 3,026 (+6) +0.20%
poseidonsponge_x5_254_inliner_min 3,174 (+6) +0.19%
regression_5252_inliner_zero 3,401 (+6) +0.18%
regression_5252_inliner_min 3,594 (+6) +0.17%
poseidonsponge_x5_254_inliner_max 4,104 (+2) +0.05%
regression_5252_inliner_max 4,444 (+2) +0.05%
poseidon_bn254_hash_inliner_zero 4,686 (+2) +0.04%
poseidon_bn254_hash_width_3_inliner_zero 4,686 (+2) +0.04%
poseidon_bn254_hash_inliner_max 5,257 (+2) +0.04%
poseidon_bn254_hash_width_3_inliner_max 5,257 (+2) +0.04%

Copy link
Contributor

github-actions bot commented Feb 14, 2025

Changes to circuit sizes

Generated at commit: 52ccaafbb99ea5dfa20ad1520d7b046eab9f75cd, compared to commit: 31cc6a1cf9ea0a02931ef60c71d4d41524f8b84c

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap +210 ❌ +0.57% +237 ❌ +0.24%
loop_invariant_regression +6 ❌ +120.00% +6 ❌ +0.22%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 36,788 (+210) +0.57% 97,792 (+237) +0.24%
loop_invariant_regression 11 (+6) +120.00% 2,771 (+6) +0.22%
sha256_var_size_regression 10,198 (+2) +0.02% 62,975 (+4) +0.01%

@vezenovm
Copy link
Contributor Author

loop_invariant_regression_inliner_max +73 ❌ +7.61%

To get these regressions back, we would need some way to determine that the predicate that the instruction is under is the same as the predicate of the loop pre-header. This is a bit simpler for ACIR after flattening and unrolling, but invariant code motion occurs before unrolling and is mostly needed for Brillig where we still have a CFG.

@vezenovm vezenovm marked this pull request as draft February 14, 2025 18:21
@vezenovm vezenovm marked this pull request as ready for review February 14, 2025 18:22
@vezenovm
Copy link
Contributor Author

ram_blowup_regression_inliner_max +43,008 ❌ +6.31%
brillig_cow_regression_inliner_max +28,236 ❌ +6.22%

Bug was introduced in this PR #6910. We look to have kept about half of the gains with this correction fix:
#6910 (comment)

@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 14, 2025

This is an issue because if we were to have an operation that can potentially error (such as from overflow or div by zero) that was under a predicate, we would hoist that instruction out of the loop. If that predicate turns off the error (e.g. by protecting against a div by zero in user code) a user would still hit an error.

Will add some more tests showing this explicitly.

EDIT: Added an additional method to execution_success/loop_invariant_regression that fails on master and passes on this PR.

@vezenovm vezenovm marked this pull request as draft February 14, 2025 18:40
@vezenovm vezenovm marked this pull request as ready for review February 14, 2025 18:53
@vezenovm
Copy link
Contributor Author

Changes to Brillig bytecode sizes

Generated at commit: b86b465b135eda14235af49815aeb03a3d5c5fa0, compared to commit: 38eeee39a98a62747dcca3b31b409151761d4ef1

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
loop_invariant_regression_inliner_max +34 ❌ +29.31%
loop_invariant_regression_inliner_zero +34 ❌ +29.31%
loop_invariant_regression_inliner_min +39 ❌ +26.90%

This is due to a new method to the loop_invariant_regression test.

@vezenovm vezenovm added the bench-show Display benchmark results on PR label Feb 14, 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.

Execution Time

Benchmark suite Current: b6b63a3 Previous: 31cc6a1 Ratio
private-kernel-inner 0.071 s 0.07 s 1.01
private-kernel-reset 0.3 s 0.298 s 1.01
private-kernel-tail 0.018 s 0.018 s 1
rollup-base-private 0.467 s 0.467 s 1
rollup-base-public 0.189 s 0.184 s 1.03
rollup-block-root 34.9 s
rollup-merge 0.007 s 0.006 s 1.17
rollup-root 0.039 s 0.039 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 Time

Benchmark suite Current: b6b63a3 Previous: 31cc6a1 Ratio
sha256_regression 0.955 s 1.01 s 0.95
regression_4709 0.748 s 0.766 s 0.98
ram_blowup_regression 20.7 s 21.2 s 0.98
global_var_regression_entry_points 0.606 s 0.618 s 0.98
private-kernel-inner 2.134 s 2.066 s 1.03
private-kernel-reset 6.458 s 6.542 s 0.99
private-kernel-tail 0.998 s 0.942 s 1.06
rollup-base-private 9.794 s 9.102 s 1.08
rollup-base-public 5.158 s 5.172 s 1.00
rollup-block-root-empty 0.958 s 0.93 s 1.03
rollup-block-root-single-tx 94.9 s 92.3 s 1.03
rollup-block-root 89 s
rollup-merge 0.927 s 0.914 s 1.01
rollup-root 1.542 s 1.524 s 1.01

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: a1af2bf Previous: 31cc6a1 Ratio
private-kernel-inner 275.62 MB 275.6 MB 1.00
private-kernel-reset 586.93 MB 586.93 MB 1
private-kernel-tail 199.2 MB 199.21 MB 1.00
rollup-base-private 950.65 MB 950.65 MB 1
rollup-base-public 816.27 MB 816.18 MB 1.00
rollup-block-root-empty 326.22 MB 326.24 MB 1.00
rollup-block-root-single-tx 6860 MB 6860 MB 1
rollup-block-root 6870 MB 6870 MB 1
rollup-merge 324.65 MB 324.65 MB 1
rollup-root 372.32 MB 372.31 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: a1af2bf Previous: 31cc6a1 Ratio
private-kernel-inner 214.85 MB 214.85 MB 1
private-kernel-reset 249.37 MB 249.37 MB 1
private-kernel-tail 184.8 MB 184.8 MB 1
rollup-base-private 431.41 MB 431.41 MB 1
rollup-base-public 370.16 MB 370.16 MB 1
rollup-block-root 1410 MB 1410 MB 1
rollup-merge 309.06 MB 309.06 MB 1
rollup-root 316.45 MB 316.45 MB 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.

Test Suite Duration

Benchmark suite Current: b6b63a3 Previous: 31cc6a1 Ratio
AztecProtocol_aztec-packages_noir-projects_aztec-nr 33 s 33 s 1
AztecProtocol_aztec-packages_noir-projects_noir-contracts 71 s 72 s 0.99
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 55 s 51 s 1.08
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 166 s 165 s 1.01
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_rollup-lib 166 s 173 s 0.96
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 56 s 55 s 1.02
noir-lang_noir-bignum_ 358 s
noir-lang_noir_bigcurve_ 255 s 288 s 0.89
noir-lang_noir_json_parser_ 12 s 12 s 1

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

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM, small nit.

@TomAFrench TomAFrench added this pull request to the merge queue Feb 18, 2025
Merged via the queue into master with commit 64890c0 Feb 18, 2025
102 checks passed
@TomAFrench TomAFrench deleted the mv/fix-binary-hoisting branch February 18, 2025 15:37
TomAFrench added a commit that referenced this pull request Feb 19, 2025
* master:
  fix(performance): Remove redundant slice access check from brillig (#7434)
  chore(docs): updating tutorials and other nits to beta.2 (#7405)
  feat: LSP hover for integer literals (#7368)
  feat(experimental): Compile match expressions (#7312)
  feat(acir_field): Add little-endian byte serialization for FieldElement (#7258)
  feat: allow unquoting TraitConstraint in trait impl position (#7395)
  feat(brillig): Hoist shared constants across functions to the global space (#7216)
  feat(LSP): auto-import via visible reexport (#7409)
  fix(brillig): Brillig entry point analysis and function specialization through duplication (#7277)
  chore: redo typo PR by maximevtush (#7425)
  fix(ssa): Accurately mark binary ops for hoisting and check Div/Mod against induction variable lower bound (#7396)
  feat!: remove bigint from stdlib (#7411)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 19, 2025
…m brillig (noir-lang/noir#7434)

chore(docs): updating tutorials and other nits to beta.2 (noir-lang/noir#7405)
feat: LSP hover for integer literals (noir-lang/noir#7368)
feat(experimental): Compile match expressions (noir-lang/noir#7312)
feat(acir_field): Add little-endian byte serialization for FieldElement (noir-lang/noir#7258)
feat: allow unquoting TraitConstraint in trait impl position (noir-lang/noir#7395)
feat(brillig): Hoist shared constants across functions to the global space (noir-lang/noir#7216)
feat(LSP): auto-import via visible reexport (noir-lang/noir#7409)
fix(brillig): Brillig entry point analysis and function specialization through duplication (noir-lang/noir#7277)
chore: redo typo PR by maximevtush (noir-lang/noir#7425)
fix(ssa): Accurately mark binary ops for hoisting and check Div/Mod against induction variable lower bound (noir-lang/noir#7396)
feat!: remove bigint from stdlib (noir-lang/noir#7411)
chore: bump aztec-packages commit (noir-lang/noir#7415)
chore: deprecate `merkle` module of stdlib (noir-lang/noir#7413)
chore(ci): lock aztec-packages commit in CI (noir-lang/noir#7414)
feat: while statement (noir-lang/noir#7280)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 19, 2025
…oir-lang/noir#7434)

chore(docs): updating tutorials and other nits to beta.2 (noir-lang/noir#7405)
feat: LSP hover for integer literals (noir-lang/noir#7368)
feat(experimental): Compile match expressions (noir-lang/noir#7312)
feat(acir_field): Add little-endian byte serialization for FieldElement (noir-lang/noir#7258)
feat: allow unquoting TraitConstraint in trait impl position (noir-lang/noir#7395)
feat(brillig): Hoist shared constants across functions to the global space (noir-lang/noir#7216)
feat(LSP): auto-import via visible reexport (noir-lang/noir#7409)
fix(brillig): Brillig entry point analysis and function specialization through duplication (noir-lang/noir#7277)
chore: redo typo PR by maximevtush (noir-lang/noir#7425)
fix(ssa): Accurately mark binary ops for hoisting and check Div/Mod against induction variable lower bound (noir-lang/noir#7396)
feat!: remove bigint from stdlib (noir-lang/noir#7411)
chore: bump aztec-packages commit (noir-lang/noir#7415)
chore: deprecate `merkle` module of stdlib (noir-lang/noir#7413)
chore(ci): lock aztec-packages commit in CI (noir-lang/noir#7414)
feat: while statement (noir-lang/noir#7280)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 19, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(performance): Remove redundant slice access check from brillig
(noir-lang/noir#7434)
chore(docs): updating tutorials and other nits to beta.2
(noir-lang/noir#7405)
feat: LSP hover for integer literals
(noir-lang/noir#7368)
feat(experimental): Compile match expressions
(noir-lang/noir#7312)
feat(acir_field): Add little-endian byte serialization for FieldElement
(noir-lang/noir#7258)
feat: allow unquoting TraitConstraint in trait impl position
(noir-lang/noir#7395)
feat(brillig): Hoist shared constants across functions to the global
space (noir-lang/noir#7216)
feat(LSP): auto-import via visible reexport
(noir-lang/noir#7409)
fix(brillig): Brillig entry point analysis and function specialization
through duplication (noir-lang/noir#7277)
chore: redo typo PR by maximevtush
(noir-lang/noir#7425)
fix(ssa): Accurately mark binary ops for hoisting and check Div/Mod
against induction variable lower bound
(noir-lang/noir#7396)
feat!: remove bigint from stdlib
(noir-lang/noir#7411)
chore: bump aztec-packages commit
(noir-lang/noir#7415)
chore: deprecate `merkle` module of stdlib
(noir-lang/noir#7413)
chore(ci): lock aztec-packages commit in CI
(noir-lang/noir#7414)
feat: while statement (noir-lang/noir#7280)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <[email protected]>
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.

3 participants