Skip to content

chore(ACIR): expand signed div and mod in SSA#10039

Merged
asterite merged 7 commits intoab/ssa-expand-signed-ltfrom
ab/ssa-expand-signed-div
Oct 1, 2025
Merged

chore(ACIR): expand signed div and mod in SSA#10039
asterite merged 7 commits intoab/ssa-expand-signed-ltfrom
ab/ssa-expand-signed-div

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Sep 30, 2025

Description

Problem

Resolves #10029

Summary

Follow-up to #10036.

I'm mainly opening this PR to see if things still work and also if this leads to an improvement.

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.

@asterite asterite changed the title Ab/ssa expand signed div chore(ACIR): expand signed div and mod in SSA Sep 30, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

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

Benchmark suite Current: 6479bc5 Previous: 39f193c Ratio
perfectly_parallel_batch_inversion_opcodes 2778569 ns/iter (± 1705) 2254940 ns/iter (± 1115) 1.23

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

CC: @TomAFrench

@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2025

Changes to circuit sizes

Generated at commit: 54cd9ebd9223c7f97fe211733a8713ed91d3bcdc, compared to commit: 612cb1ea99958991d3e06e4b3bc7de712cbd1706

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
signed_div -45 ✅ -7.89% -45 ✅ -7.49%
regression_9971 -5 ✅ -12.50% -30 ✅ -22.73%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
signed_inactive_division_by_zero 46 (+2) +4.55% 2,813 (+2) +0.07%
inactive_signed_bitshift 79 (+2) +2.60% 2,842 (+1) +0.04%
bit_shifts_comptime 51 (-10) -16.39% 2,906 (-8) -0.27%
signed_arithmetic 178 (-11) -5.82% 2,933 (-11) -0.37%
regression_10008 13 (-7) -35.00% 2,774 (-11) -0.39%
regression_8726 51 (-20) -28.17% 2,888 (-23) -0.79%
signed_division 184 (-42) -18.58% 3,680 (-37) -1.00%
bit_shifts_runtime 467 (-60) -11.39% 4,714 (-51) -1.07%
regression_1144_1169_2399_6609 254 (-69) -21.36% 5,172 (-59) -1.13%
signed_div 525 (-45) -7.89% 556 (-45) -7.49%
regression_9971 35 (-5) -12.50% 102 (-30) -22.73%

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: 6479bc5 Previous: 39f193c Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 346 s 257 s 1.35
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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

CC: @TomAFrench

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.

Looks good! Just some nits

@asterite
Copy link
Collaborator Author

asterite commented Oct 1, 2025

One thing I forgot to mention here is that I think signed division/modulo end up a bit more optimal is because previously we would check that -128 i8 / -1 i8 didn't happen by some condition on the quotient, remainder and predicate. Here we check that the input values aren't those. My guess is that if the right-hand side is a constant and not -1 then the check can be avoided, while in the previous case it was included. Or at least when I compare the ACIR before and after, dividing by a constant, the ACIR ends up being shorter. Well, I think that's just one reason, maybe the smaller SSA instructions end up optimized in more ways, not sure.

@vezenovm
Copy link
Contributor

vezenovm commented Oct 1, 2025

My guess is that if the right-hand side is a constant and not -1 then the check can be avoided, while in the previous case it was included. Or at least when I compare the ACIR before and after, dividing by a constant, the ACIR ends up being shorter. Well, I think that's just one reason, maybe the smaller SSA instructions end up optimized in more ways, not sure.

I imagine we are optimizing out the instructions. In ACIR gen we don't have any optimization opportunities other than those in the ACVM while in our SSA pipeline we have many more.

@asterite
Copy link
Collaborator Author

asterite commented Oct 1, 2025

Since this and the base PR are approved, I'll merge this and then merge the base PR.

@asterite asterite merged commit 0c0e296 into ab/ssa-expand-signed-lt Oct 1, 2025
127 of 128 checks passed
@asterite asterite deleted the ab/ssa-expand-signed-div branch October 1, 2025 15:15
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