-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ssa): Expand feature set of the Brillig constraint check #7060
base: master
Are you sure you want to change the base?
Conversation
Execution Memory Report
|
Compilation Memory Report
|
Compilation Report
|
Execution Report
|
Indeed, would have to look into it |
compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs
Show resolved
Hide resolved
compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs
Outdated
Show resolved
Hide resolved
compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs
Show resolved
Hide resolved
compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs
Show resolved
Hide resolved
Should be all of them |
…ined_values.rs Co-authored-by: Michael J Klein <[email protected]>
…xes' into rk/brillig-check-fixes
There was a problem hiding this 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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20
.
Benchmark suite | Current: 2d2a32e | Previous: c8d5ce5 | Ratio |
---|---|---|---|
rollup-base-public |
40.82 s |
27.2 s |
1.50 |
rollup-base-private |
14.14 s |
10.32 s |
1.37 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
@TomAFrench should we merge with the current (greatly reduced but still out there) performance hit? I've dealt with the main culprit which turned out to be an actual bug but the check is still slower now just because of increased complexity. I believe I can bring it further down with some optimization, or otherwise make certain new features optional. The question is, do we need that in this PR or can it be taken care of separately? |
Description
This PR resolves a number of issues that came up in the initial introduction of the Brillig constraint check, expanding its feature set.
Problem
#6698, #6706, #6713, #6735
Summary
Resolves #6698 by creating and maintaining additional tainted value sets for every result array element, #6713 by tracking EnableSideEffectsIf condition values, and #6735 along with #6706 by introducing a quick pass gathering information on Brillig calls. New tests for every issue are included.
As fixing #6698 makes the
check_underconstrained_regression
test program (correctly) issue a bug warning now, the program has been updated to properly constrain the involved array elements.Additional Context
Documentation
Check one:
PR Checklist*
cargo fmt
on default settings.