chore: do not inline acir calls in brillig #9412
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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
| @@ -552,7 +564,8 @@ impl<'function> PerFunctionContext<'function> { | |||
| match callee.runtime() { | |||
| RuntimeType::Acir(inline_type) => { | |||
| // If the called function is acir, we inline if it's not an entry point | |||
| if inline_type.is_entry_point() { | |||
| // If it is called from brillig, it cannot be inlined because runtimes do not share the same semantic | |||
| if inline_type.is_entry_point() || self.entry_function.runtime().is_brillig() { | |||
There was a problem hiding this comment.
One of the audit issues #9404 is about moving away from both this should_inline_call and the separate closure should_inline_call that checks the inline infos. Could we try to prevent inline ACIR into Brillig using that?
There was a problem hiding this comment.
I guess as we are already looking at the entry function here this is fine for now and we can converge the should inline check in a separate PR.
|
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. |
There was a problem hiding this comment.
⚠️ 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
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. |
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
compiler/noirc_evaluator/src/ssa/opt/inline_simple_functions.rs
Outdated
Show resolved
Hide resolved
|
Shall we change the base of this PR to go into @jfecher's branch? |
|
@guipublic #7289 is resolved so this PR should be ready again once merge conflicts are resolved. |
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:
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:
PR Checklist*
cargo fmton default settings.