Skip to content

chore: greenglight remove_ureachable_instructions#9689

Merged
asterite merged 8 commits intomasterfrom
ab/greenlight_remove_unreachable_instructions
Sep 2, 2025
Merged

chore: greenglight remove_ureachable_instructions#9689
asterite merged 8 commits intomasterfrom
ab/greenlight_remove_unreachable_instructions

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves #9518

Summary

This just add docs.

I documented how it works but I'm not sure why:

  • for array operations we don't put a constrain failure instead of creating a dummy array and fetching from it
  • for slice operations we create a zeroed slice and return it when we already have a guaranteed empty slice (the one that produces the failure) so we could continue with that slice instead of create a new, empty one

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.

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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 8f1d9ce Previous: e5bd638 Ratio
rollup-block-root-empty 30.58 s 21.84 s 1.40

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

CC: @TomAFrench

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: b5eb4d7 Previous: 19f2525 Ratio
test_report_noir-lang_noir_bigcurve_ 393 s 326 s 1.21

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

CC: @TomAFrench

@asterite asterite requested a review from a team August 29, 2025 20:06
@vezenovm
Copy link
Contributor

vezenovm commented Sep 2, 2025

  • for array operations we don't put a constrain failure instead of creating a dummy array and fetching from it

This is because we gate the check only for ACIR. ACIR array operations are side effectual and check for array OOBs, while in Brillig they do not so we would have to insert a constrain in the Brillig runtime #9378. However, this was breaking due to #7289 still being open at the time. I want to hold off on merging #9378 though as it looks like we are getting failures in aztec-packages from #9412 still.

  • for slice operations we create a zeroed slice and return it when we already have a guaranteed empty slice (the one that produces the failure) so we could continue with that slice instead of create a new, empty one

This is a good point. We could change this logic and slightly simplify that case.

@asterite
Copy link
Collaborator Author

asterite commented Sep 2, 2025

This is a good point. We could change this logic and slightly simplify that case.

In the end I'm not sure this is worth it. The slice operations return the slice but also the popped element so we'd have to zero that anyway.

@vezenovm
Copy link
Contributor

vezenovm commented Sep 2, 2025

The slice operations return the slice but also the popped element so we'd have to zero that anyway.

We could just add that as an inline comment as an extra justification.

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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: d609445 Previous: 23ba35f Ratio
rollup-root 0.005 s 0.004 s 1.25

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

CC: @TomAFrench

asterite and others added 3 commits September 2, 2025 14:09
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
@asterite asterite enabled auto-merge September 2, 2025 17:10
@asterite asterite added this pull request to the merge queue Sep 2, 2025
Merged via the queue into master with commit 7cab4ea Sep 2, 2025
171 of 173 checks passed
@asterite asterite deleted the ab/greenlight_remove_unreachable_instructions branch September 2, 2025 20:08
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.

Greenlight remove_unreachable_instructions for audits

2 participants