Skip to content

Conversation

guipublic
Copy link
Contributor

@guipublic guipublic commented Aug 6, 2025

Description

Problem*

Resolves #9390

Summary*

ACIR calls from Brillig are not inlined and furthermore they are forbidden during ssa validation.

Additional Context

The defunctionalisation pass has also been updated, because of these problems that become visible with this PR:

  • the signatures when computing the 'variants' (i.e the set of functions matching a signature) are not checked against their runtime.
  • generated code for apply function crash if there is a runtime error, due to not having a location.
  • empty variants get a dummy function.

It turns out that calling acir from brillig can happen in the apply function of the defunctionalisation pass: this happens for instance with the zeroed functions generated by the frontend in the lambda_from_dynamic_if test, with force-brillig.
In that case, generating a runtime error when calling acir from brillig generates a crash due to missing location.
Filtering the bad function in the 'variants' works in this case but fail in other cases due to the dummy function.

As a result, I ensured that the apply function never calls acir from brillig by adding an assert if it happens, which provides an error message to the user.

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.

@guipublic guipublic requested a review from vezenovm August 6, 2025 12:46
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 Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 6d23b0a Previous: b2713a9 Ratio
sha512_100_bytes 137.5 MB 54.97 MB 2.50

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

CC: @TomAFrench

@TomAFrench
Copy link
Member

I remember adding some assertions for this case in the past and from what I remember, I ran into issues with closures. I was working on the understanding that we'd need #7289 before we could enforce this.

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: a422e94 Previous: 3679e4c Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 1 s 3

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

CC: @TomAFrench

@guipublic
Copy link
Contributor Author

I remember adding some assertions for this case in the past and from what I remember, I ran into issues with closures. I was working on the understanding that we'd need #7289 before we could enforce this.

Yes this is what is happening here, most tests with lambdas have the issue (most of the case with sorting arrays). At least it is now explicit.

Copy link
Contributor

github-actions bot commented Aug 7, 2025

Changes to Brillig bytecode sizes

Generated at commit: ee564298e0b2681546ded1d4a578c5fe8d1d7798, compared to commit: f0783eaec0ba97f6f2147ace59f6f787dc55d322

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
lambda_from_dynamic_if_inliner_min -5 ✅ -0.98%
lambda_from_dynamic_if_inliner_zero -5 ✅ -1.00%

Full diff report 👇
Program Brillig opcodes (+/-) %
lambda_from_dynamic_if_inliner_max 608 (-5) -0.82%
lambda_from_dynamic_if_inliner_min 504 (-5) -0.98%
lambda_from_dynamic_if_inliner_zero 496 (-5) -1.00%

@TomAFrench
Copy link
Member

Shall we change the base of this PR to go into @jfecher's branch?

@vezenovm
Copy link
Contributor

Shall we change the base of this PR to go into @jfecher's branch?

This PR would be useful for testing out #9484.

@vezenovm
Copy link
Contributor

@guipublic #7289 is resolved so this PR should be ready again once merge conflicts are resolved.

@vezenovm vezenovm enabled auto-merge September 4, 2025 13:48
@vezenovm vezenovm added this pull request to the merge queue Sep 4, 2025
Merged via the queue into master with commit 334be14 Sep 4, 2025
120 of 122 checks passed
@vezenovm vezenovm deleted the gd/issue_9390 branch September 4, 2025 14:34
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.

Inlining ACIR into Brillig can trigger incorrect semantics
3 participants