Skip to content

chore: store function purities on the SSA#9357

Closed
asterite wants to merge 12 commits intomasterfrom
ab/no-purities-on-dfgs
Closed

chore: store function purities on the SSA#9357
asterite wants to merge 12 commits intomasterfrom
ab/no-purities-on-dfgs

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Jul 30, 2025

Description

Problem

Made on top of #9355, just trying something.

Summary

This is just an experiment to see how the code looks like if we avoid storing function purities on every function dfg and instead store it once in the SSA and pass it around as needed.

I'm not 100% this is worth it, but it does seem to make the code a bit easier to follow (we can't forget to set purities in some functions, etc.), so I'm mainly interested in your opinion.

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

Benchmark suite Current: f20a89e Previous: b61f5eb Ratio
test_report_zkpassport_noir-ecdsa_ 2 s 1 s 2

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

CC: @TomAFrench

@asterite asterite requested a review from a team July 30, 2025 17:16
Comment on lines +130 to +133
struct Context<'brillig, 'purities> {
purities: &'purities FunctionPurities,
use_constraint_info: bool,
brillig_info: Option<BrilligInfo<'a>>,
brillig_info: Option<BrilligInfo<'brillig>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get away with the single lifetime here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably that the additional 'purities lifetime is not necessary here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done!

asterite and others added 3 commits August 4, 2025 13:01
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@TomAFrench
Copy link
Member

This looks pretty stale at this point. @asterite shall we close this or is it worth reviving?

@asterite
Copy link
Collaborator Author

asterite commented Nov 3, 2025

Let's close this. We could always revive it later. I feel like it might be worth it because there was at least one bug where we forgot to copy purities.

@asterite asterite closed this Nov 3, 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

Development

Successfully merging this pull request may close these issues.

3 participants