Skip to content

fix: avoid division and modulo hoisting around side effect variables#8423

Closed
guipublic wants to merge 1 commit intomasterfrom
gd/issue_8261
Closed

fix: avoid division and modulo hoisting around side effect variables#8423
guipublic wants to merge 1 commit intomasterfrom
gd/issue_8261

Conversation

@guipublic
Copy link
Contributor

Description

Problem*

Resolves #8261

Summary*

Prevent division and modulo hoisting from enable side effects

Additional Context

Division or modulo are implemented using non-determinism and computed during witness generation by an internal brillig function. As a result, the function output 0 values when run under a false predicate, so it cannot be moved across enable side effects.

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 May 8, 2025

Changes to circuit sizes

Generated at commit: 35e3501474b34852692c7dfcdf14118200c5b9d9, compared to commit: 8ab59da0020d6d84ecb06d72f0f3ca073bf11f70

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression +4 ❌ +1.38% -2 ✅ -0.05%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression 294 (+4) +1.38% 3,836 (-2) -0.05%

@vezenovm
Copy link
Contributor

vezenovm commented May 8, 2025

This is fixed here #8355 where I moved to Instruction::requires_acir_gen_predicate which has Div/Mod correctly marked.

I will add this regression test there as well.

@vezenovm
Copy link
Contributor

vezenovm commented May 9, 2025

This change is contained in #8355 which has now been merged.

@vezenovm vezenovm closed this May 9, 2025
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.

Only ACIR: failed constraint (modulo, println)

2 participants