Skip to content

chore: move unsigned overflow check from acir/brillig to ssa#8163

Merged
asterite merged 13 commits intomasterfrom
ab/ssa-checked-to-unchecked
Apr 23, 2025
Merged

chore: move unsigned overflow check from acir/brillig to ssa#8163
asterite merged 13 commits intomasterfrom
ab/ssa-checked-to-unchecked

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Apr 22, 2025

Description

Problem

Resolves #8162

Summary

Additional Context

When inserting binary operations into the SSA we currently transform them a bit. For example:

  1. if we have unchecked_mul between fields we turn that to mul because checked/unchecked doesn't make sense for fields
  2. if we have unchecked_mul between bools we turn that into mul because it can never overflow

We did this to reduce the noise in the SSA, so that we don't get to see unchecked_mul between fields and wonder why it's unchecked if it doesn't make sense for it to be unchecked.

But... like that, and especially for point 2, it means that boolean multiplication will be checked unless we don't check it specifically for bools.

So... I wonder if instead unchecked_mul should be used whenever two values are being multiplied and they can never overflow. That means that mul between fields doesn't make sense anymore, and unchecked_mul is what makes sense. And the same applies for bools.

But... for now in this PR I just made those checked operations unchecked.


A next step, which I'll try as a follow-up PR, is to try #8111 again but this time memoizing the maximum bit size of values as we traverse the instructions (to avoid recursing multiple times, maybe that was the cause of the slowdown).


Yet another thought: unchecked: false is kind of hard to understand because it's a double negation. I wonder if renaming unchecked to checked and flipping all bools would make things a bit clearer.

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.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 22, 2025

Changes to circuit sizes

Generated at commit: 8e20ac596288b2f60228b1e6a48e484c3fdf4f6c, compared to commit: ae008d31e289ddb086b4c69e3fd287c5167fc4dc

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap -8 ✅ -0.02% -8 ✅ -0.01%
array_oob_regression_7965 -8 ✅ -20.51% -8 ✅ -0.28%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 33,469 (-8) -0.02% 94,499 (-8) -0.01%
array_oob_regression_7965 31 (-8) -20.51% 2,843 (-8) -0.28%

@asterite
Copy link
Collaborator Author

Cool, it went from a regression to an optimization! (not sure why, though, but maybe unchecked operations have more consequences along the way...)

@asterite asterite requested a review from a team April 22, 2025 19:29
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.

LGTM, I have wanted to move more of this shared logic into the SSA so happy to see it!

@asterite asterite enabled auto-merge April 23, 2025 14:55
@asterite asterite added this pull request to the merge queue Apr 23, 2025
Merged via the queue into master with commit ccbaf3d Apr 23, 2025
113 of 114 checks passed
@asterite asterite deleted the ab/ssa-checked-to-unchecked branch April 23, 2025 15:23
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
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.

chore: move checking of unsigned overflow from acir-gen/brillig-gen to SSA

3 participants