Skip to content
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

fix(ssa): Make the lookback feature opt-in #7190

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rkarabut
Copy link
Contributor

@rkarabut rkarabut commented Jan 25, 2025

Description

Problem

The Brillig constraints check can get slow on large rollout functions, with the recently added lookback feature, while allowing to catch some rare false positives, looking to be the main culprit.

Summary

This PR disables lookback by default and allows the developer to employ it at their discretion. Also, repeated code locations of brillig calls are filtered out, improving performance of the pass.

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.

@rkarabut rkarabut requested a review from TomAFrench January 25, 2025 03:18
@TomAFrench
Copy link
Member

Even prior to the addition of this feature, this pass was causing compilation to hang on the protocol circuits so I'm in favour of making the entire pass opt in.

@rkarabut
Copy link
Contributor Author

Even prior to the addition of this feature, this pass was causing compilation to hang on the protocol circuits so I'm in favour of making the entire pass opt in.

Alright. Gotta put some additional research into this, as in doing test runs working on the initial check protocol circuits worked fine for me (although I've had word of the hangups later). Removed "resolves #7171" from the description. I think the feature should be opt-in any case though.

@rkarabut rkarabut linked an issue Jan 26, 2025 that may be closed by this pull request
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: dfec078 Previous: 130d991 Ratio
noir-lang_noir_check_shuffle_ 1 s 0 s +∞
noir-lang_ec_ 1 s 0 s +∞

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

CC: @TomAFrench

@rkarabut
Copy link
Contributor Author

rkarabut commented Feb 4, 2025

Added filtering for already visited code locations of the brillig calls, which seems to help a lot with huge unrolled functions. Probably could switch the check back as the default.

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