Skip to content

fix: use range-constrained var from unsigned_overflow_check#8572

Closed
asterite wants to merge 3 commits intomasterfrom
ab/use-check-unsigned-overflow-predicated-variable
Closed

fix: use range-constrained var from unsigned_overflow_check#8572
asterite wants to merge 3 commits intomasterfrom
ab/use-check-unsigned-overflow-predicated-variable

Conversation

@asterite
Copy link
Copy Markdown
Collaborator

@asterite asterite commented May 19, 2025

Description

Problem

Resolves #8556
Resolves #8557
Resolves #8558

Summary

Applied this suggestions from @guipublic :

I think the solution should be to use the 'predicated' variable (predicate_range) in check_unsigned_overflow() as the result of the binary instruction. That should help for other things as well (e.g avoiding predicates in division by constant).
(The alternative is to use predicate when doing the truncation)

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 force-pushed the ab/use-check-unsigned-overflow-predicated-variable branch from 27ad8f4 to af14c01 Compare May 19, 2025 18:40
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 19, 2025

Changes to circuit sizes

Generated at commit: cd01741ab491d2f7eb233a53612e2c0b7bf856cb, compared to commit: 4972733638fa117f84848a3d923df1724bd84821

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
conditional_1 -55 ✅ -2.05% -59 ✅ -0.73%
regression_8329 -3 ✅ -7.69% -3 ✅ -5.45%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
slices 670 (+2) +0.30% 3,769 (+2) +0.05%
regression_3607 37 (0) 0.00% 2,800 (+1) +0.04%
hashmap 33,473 (+4) +0.01% 94,508 (+4) +0.00%
slice_dynamic_index 812 (0) 0.00% 6,464 (-1) -0.02%
conditional_1 2,633 (-55) -2.05% 7,987 (-59) -0.73%
regression_8329 36 (-3) -7.69% 52 (-3) -5.45%

@asterite asterite marked this pull request as ready for review May 19, 2025 19:19
@asterite asterite requested a review from guipublic May 19, 2025 19:19
@asterite
Copy link
Copy Markdown
Collaborator Author

@guipublic Is this what you had in mind?

If so, I wonder if the variable returned from range_constrain_var should be used in more places. Right now it's ignored in most cases.

@asterite
Copy link
Copy Markdown
Collaborator Author

Closed in favor of #8583
/

@asterite asterite closed this May 20, 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

1 participant