Skip to content

fix: mark all signed binary ops as unchecked#9059

Closed
TomAFrench wants to merge 2 commits intomasterfrom
tf/mark-signed-ops-as-unchecked
Closed

fix: mark all signed binary ops as unchecked#9059
TomAFrench wants to merge 2 commits intomasterfrom
tf/mark-signed-ops-as-unchecked

Conversation

@TomAFrench
Copy link
Member

Description

Problem*

Resolves #9047

Summary*

Shows a fix for #9047. I think that we probably want to go a bit further and push the overflow checks for the signed operations inside of the actual binary instruction. This can then have an SSA pass which expands these into the current format but we should avoid the jankiness which currently exists around signed types.

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 30, 2025

Changes to Brillig bytecode sizes

Generated at commit: 8e52c9c8271fe2795e9fae47376dcb48e229a27d, compared to commit: d6920017a3b1ae0a1c927e50b01ca748d901e9ce

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
higher_order_functions_inliner_zero -1 ✅ -0.15%

Full diff report 👇
Program Brillig opcodes (+/-) %
higher_order_functions_inliner_zero 651 (-1) -0.15%

@github-actions
Copy link
Contributor

github-actions bot commented Jun 30, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 8e52c9c8271fe2795e9fae47376dcb48e229a27d, compared to commit: d6920017a3b1ae0a1c927e50b01ca748d901e9ce

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
higher_order_functions_inliner_zero +6 ❌ +0.51%

Full diff report 👇
Program Brillig opcodes (+/-) %
higher_order_functions_inliner_zero 1,192 (+6) +0.51%

@TomAFrench
Copy link
Member Author

hmm, interesting. I didn't expect this to affect brillig outputs for existing programs 🤔

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: 030669b Previous: 3838c69 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 101 s 75 s 1.35
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 153 s 127 s 1.20
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 173 s 141 s 1.23
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 95 s 73 s 1.30

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

CC: @TomAFrench

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.

Min != Full: Should never execute, yet degenerates into always-fail

1 participant