Skip to content

fix(ssa): Do not deduplicate division by a zero constant#7393

Merged
TomAFrench merged 1 commit intomasterfrom
mv/do-not-deduplicate-div-by-const-zero
Feb 14, 2025
Merged

fix(ssa): Do not deduplicate division by a zero constant#7393
TomAFrench merged 1 commit intomasterfrom
mv/do-not-deduplicate-div-by-const-zero

Conversation

@vezenovm
Copy link
Copy Markdown
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
@github-actions
Copy link
Copy Markdown
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
Copy Markdown
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
@TomAFrench TomAFrench deleted the mv/do-not-deduplicate-div-by-const-zero branch February 14, 2025 17:40
@TomAFrench
Copy link
Copy Markdown
Member

TomAFrench commented Feb 14, 2025

image

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

AztecBot pushed 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 pushed 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