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): Do not deduplicate division by a zero constant #7393

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 14, 2025

Description

Problem*

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

Summary*

If the rhs of a div/mod was zero we would return false from requires_acir_gen_predicate. This would result in deduplicating a div/mod by a zero constant.
e.g. this SSA:

        acir(inline) fn main f0 {
          b0(v0: u32, v1: u32, v2: u1):
            enable_side_effects v2
            v4 = div v1, u32 0
            v5 = not v2
            enable_side_effects v5
            v6 = div v1, u32 0
            return
        }

would turn into this SSA:

        acir(inline) fn main f0 {
          b0(v0: u32, v1: u32, v2: u1):
            enable_side_effects v2
            v4 = div v1, u32 0
            v5 = not v2
            enable_side_effects v5
            return
        }

If v2 was inactive and v5 was active, we would miss needing to fail when processing the second division.

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 TomAFrench February 14, 2025 17:17
Copy link
Contributor

Changes to circuit sizes

Generated at commit: 57e9776b31b7278500bf6457c3ac4cb8151fb192, compared to commit: 2391a8ef05498bac0d7d601c4db79b0621ca3339

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression -12 ✅ -4.11% -16 ✅ -0.42%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression 280 (-12) -4.11% 3,811 (-16) -0.42%

@TomAFrench
Copy link
Member

Nice catch

@TomAFrench TomAFrench enabled auto-merge February 14, 2025 17:25
@TomAFrench TomAFrench added this pull request to the merge queue Feb 14, 2025
Merged via the queue into master with commit 38eeee3 Feb 14, 2025
103 checks passed
@TomAFrench TomAFrench deleted the mv/do-not-deduplicate-div-by-const-zero branch February 14, 2025 17:40
@TomAFrench
Copy link
Member

TomAFrench commented Feb 14, 2025

image

Looks like we're doing a lot of duplicate divisions by constants!

AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 14, 2025
…oir-lang/noir#7393)

chore: document traits required to be in scope (noir-lang/noir#7387)
fix: field zero division in brillig (noir-lang/noir#7386)
chore: box `ParserError`s in `InterpreterError` (noir-lang/noir#7373)
chore: remove unnecessary dereferencing within brillig vm (noir-lang/noir#7375)
fix: give "correct" error when trying to use AsTraitPath (noir-lang/noir#7360)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 14, 2025
…oir#7393)

chore: document traits required to be in scope (noir-lang/noir#7387)
fix: field zero division in brillig (noir-lang/noir#7386)
chore: box `ParserError`s in `InterpreterError` (noir-lang/noir#7373)
chore: remove unnecessary dereferencing within brillig vm (noir-lang/noir#7375)
fix: give "correct" error when trying to use AsTraitPath (noir-lang/noir#7360)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 14, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(ssa): Do not deduplicate division by a zero constant
(noir-lang/noir#7393)
chore: document traits required to be in scope
(noir-lang/noir#7387)
fix: field zero division in brillig
(noir-lang/noir#7386)
chore: box `ParserError`s in `InterpreterError`
(noir-lang/noir#7373)
chore: remove unnecessary dereferencing within brillig vm
(noir-lang/noir#7375)
fix: give "correct" error when trying to use AsTraitPath
(noir-lang/noir#7360)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
TomAFrench added a commit that referenced this pull request Feb 14, 2025
…rom-brillig

* master:
  chore: allow opting in to displaying benchmark comments (#7399)
  chore: box `ExprValue` in `Value` enum (#7388)
  chore: pull out refactored methods from u128 branch (#7385)
  feat: require safety comments instead of safety doc comments (#7295)
  fix(ssa): Do not deduplicate division by a zero constant (#7393)
  chore: document traits required to be in scope (#7387)
  fix: field zero division in brillig (#7386)
  chore: box `ParserError`s in `InterpreterError` (#7373)
  chore: remove unnecessary dereferencing within brillig vm (#7375)
  fix: give "correct" error when trying to use AsTraitPath (#7360)
  chore: avoid u128s in brillig memory (#7363)
  chore: update docs about integer overflows (#7370)
  fix!: Only decrement the counter of an array if its address has not changed (#7297)
  fix: let LSP read `noirfmt.toml` for formatting files (#7355)
  chore: deprecate keccak256 (#7361)
  feat: `FunctionDefinition::as_typed_expr` (#7358)
  feat(performance): Check sub operations against induction variables (#7356)
  chore: avoid doing all brillig integer arithmetic on u128s (#7357)
TomAFrench added a commit that referenced this pull request Feb 14, 2025
* master:
  chore: allow opting in to displaying benchmark comments (#7399)
  chore: box `ExprValue` in `Value` enum (#7388)
  chore: pull out refactored methods from u128 branch (#7385)
  feat: require safety comments instead of safety doc comments (#7295)
  fix(ssa): Do not deduplicate division by a zero constant (#7393)
  chore: document traits required to be in scope (#7387)
  fix: field zero division in brillig (#7386)
  chore: box `ParserError`s in `InterpreterError` (#7373)
  chore: remove unnecessary dereferencing within brillig vm (#7375)
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