feat: safe division has no side effects by asterite · Pull Request #11009 · noir-lang/noir

asterite · 2025-12-23T14:27:41Z

Description

Problem

Resolves #10985

Summary

Additional Context

We could probably unify requires_acir_gen_predicate and has_side_effects for Binary, but for now I decided to keep them slightly separate as there's a small difference.

User Documentation

Check one:

No user documentation needed.
Changes in docs/ included in this PR.
[For Experimental Features] Changes in docs/ 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 · 2025-12-23T14:32:42Z

Changes to number of Brillig opcodes executed

Generated at commit: 8fb4867b500ce7c4bbddceaf5d4059ceb6d1845f, compared to commit: ab6857626bd0764da6d76cdd2f620b7988cb8625

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_5252_inliner_zero	-40 ✅	-0.00%
loop_break_regression_8319_inliner_min	-82 ✅	-12.17%

Full diff report 👇

Program	Brillig opcodes (+/-)	%
poseidon_bn254_hash_width_3_inliner_min	168,226 (-6)	-0.00%
poseidonsponge_x5_254_inliner_min	184,993 (-8)	-0.00%
regression_5252_inliner_min	923,265 (-40)	-0.00%
poseidonsponge_x5_254_inliner_zero	183,088 (-8)	-0.00%
regression_5252_inliner_zero	911,385 (-40)	-0.00%
loop_break_regression_8319_inliner_min	592 (-82)	-12.17%

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `0391662`	Previous: `ab68576`	Ratio
`test_report_zkpassport_noir-ecdsa_`	`3` s	`2` s	`1.50`

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

CC: @TomAFrench

compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs

compiler/noirc_evaluator/src/ssa/ir/instruction.rs

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `0391662`	Previous: `ab68576`	Ratio
`rollup-checkpoint-merge`	`0.004` s	`0.003` s	`1.33`

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

CC: @TomAFrench

compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs

vezenovm · 2025-12-23T18:11:05Z

compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs

          b0(v0: u1):
            enable_side_effects v0
            constrain u1 0 == v0, "attempt to divide by zero"
+            v3 = div u64 1, u64 1


Hmm I still would expect this instruction to be removed.

Me too. I was being lazy here. Now I re-checked this and it seems the logic is that instructions that have side effects are replaced with instructions with default values, while ones that don't have side effects are left as-is. But... I don't understand why. I don't know why instructions are never removed in this case.

Ah, nevermind, instructions are removed, but only those that have side effects. Maybe it's because we under the effects of a predicate? But I still don't fully understand this.

Yes it doesn't make sense to me that an instruction without side effects wouldn't be removed...

This seems like it may even be a bug. I would expect instructions without side effects to just be removed.

Let's just do #11009 (comment) to prevent these changes and we can determine why this is happening in follow-up work.

We can probably wait for Akosh to come back, I think he did this part.

vezenovm · 2025-12-23T18:13:54Z

compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs

                    // Check if this operation is one that should only fail if the predicate is enabled.
-                    let requires_acir_gen_predicate =
-                        binary.requires_acir_gen_predicate(context.dfg);
+                    let requires_acir_gen_predicate = binary.has_side_effects(context.dfg);


Couldn't we still call requires_acir_gen_predicate here which calls has_side_effects internally? Do we maybe we need to override the Div/Mod cases in requires_acir_gen_predicate?

Couldn't we still call requires_acir_gen_predicate here which calls has_side_effects internally?

No, because it's gone. Or should we keep requires_acir_gen_predicate as a synonym of has_side_effects for Binary?

Or should we keep requires_acir_gen_predicate as a synonym of has_side_effects for Binary?

Yeah that is what I was thinking. And then before we call out to has_side_effects (inside of requires_acir_gen_predicate) we can special case Div/Mod to return true like they currently do. In a follow-up we can then address #11009 (comment) as it is starting to leave the scope of this PR.

…ctions.rs Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>

github-actions · 2025-12-23T18:52:02Z

Changes to circuit sizes

Generated at commit: 8fb4867b500ce7c4bbddceaf5d4059ceb6d1845f, compared to commit: ab6857626bd0764da6d76cdd2f620b7988cb8625

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
regression_1144_1169_2399_6609	-20 ✅	-8.03%	-33 ✅	-0.63%
regression_9971	-5 ✅	-14.29%	-8 ✅	-4.91%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
regression_8261	34 (-1)	-2.86%	2,880 (-2)	-0.07%
regression_10008	11 (-2)	-15.38%	2,812 (-4)	-0.14%
inactive_signed_bitshift	76 (-3)	-3.80%	2,877 (-8)	-0.28%
signed_inactive_division_by_zero	42 (-4)	-8.70%	2,845 (-10)	-0.35%
regression_6834	26 (-5)	-16.13%	2,877 (-13)	-0.45%
regression_1144_1169_2399_6609	229 (-20)	-8.03%	5,169 (-33)	-0.63%
regression_9971	30 (-5)	-14.29%	155 (-8)	-4.91%

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `0391662`	Previous: `ab68576`	Ratio
`rollup-checkpoint-merge`	`0.002` s	`0.001` s	`2`

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

CC: @TomAFrench

feat: safe division has no side effects

6ad4c86

github-actions bot reviewed Dec 23, 2025

View reviewed changes

asterite requested a review from a team December 23, 2025 14:56

vezenovm reviewed Dec 23, 2025

View reviewed changes

asterite added 2 commits December 23, 2025 14:28

duplicate -> deduplicate

d0afd9b

Replace requires_acir_gen_predicate with has_side_effects

c749a05

github-actions bot reviewed Dec 23, 2025

View reviewed changes

asterite enabled auto-merge December 23, 2025 18:02

vezenovm reviewed Dec 23, 2025

View reviewed changes

Update compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instru…

7ac2f61

…ctions.rs Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>

Merge branch 'master' into ab/safe-div

95cee42

github-actions bot reviewed Jan 13, 2026

View reviewed changes

TomAFrench added 2 commits January 13, 2026 14:39

chore: update snapshots

c13d7f8

Merge branch 'master' into ab/safe-div

0391662

Conversation

asterite commented Dec 23, 2025

Description

Problem

Summary

Additional Context

User Documentation

PR Checklist

Uh oh!

github-actions bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to number of Brillig opcodes executed

🧾 Summary (10% most significant diffs)

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vezenovm Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vezenovm Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

github-actions bot commented Dec 23, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

vezenovm Dec 23, 2025 •

edited

Loading

vezenovm Dec 23, 2025 •

edited

Loading

github-actions bot commented Dec 23, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading