Conversation
… always be inlined
There was a problem hiding this comment.
ACVM Benchmarks
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
purely_sequential_opcodes |
255409 ns/iter (± 1185) |
253639 ns/iter (± 322) |
1.01 |
perfectly_parallel_opcodes |
225339 ns/iter (± 3433) |
222904 ns/iter (± 9810) |
1.01 |
perfectly_parallel_batch_inversion_opcodes |
3212357 ns/iter (± 4012) |
3564404 ns/iter (± 6706) |
0.90 |
This comment was automatically generated by workflow using github-action-benchmark.
…ine' into mv/mark-more-simple-funcs-to-inline
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
private-kernel-inner |
2.372 s |
2.314 s |
1.03 |
private-kernel-reset |
6.782 s |
8.546 s |
0.79 |
private-kernel-tail |
1.088 s |
1.066 s |
1.02 |
rollup-base-private |
16.08 s |
16.04 s |
1.00 |
rollup-base-public |
12.86 s |
12.88 s |
1.00 |
rollup-block-root-empty |
1.364 s |
1.306 s |
1.04 |
rollup-block-root-single-tx |
125 s |
125 s |
1 |
rollup-block-root |
128 s |
121 s |
1.06 |
rollup-merge |
1.132 s |
1.098 s |
1.03 |
rollup-root |
1.776 s |
1.7 s |
1.04 |
semaphore-depth-10 |
0.835 s |
0.836 s |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Artifact Size
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
private-kernel-inner |
1127.1 KB |
1127.1 KB |
1 |
private-kernel-reset |
2051.1 KB |
2051.1 KB |
1 |
private-kernel-tail |
583.1 KB |
583.1 KB |
1 |
rollup-base-private |
5123.1 KB |
5123.2 KB |
1.00 |
rollup-base-public |
3945.1 KB |
3941.8 KB |
1.00 |
rollup-block-root-empty |
256.8 KB |
256.8 KB |
1 |
rollup-block-root-single-tx |
25674.8 KB |
25679 KB |
1.00 |
rollup-block-root |
25714.3 KB |
25706 KB |
1.00 |
rollup-merge |
181.7 KB |
181.7 KB |
1 |
rollup-root |
477.8 KB |
473.4 KB |
1.01 |
semaphore-depth-10 |
636.3 KB |
636.3 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Time
Details
| Benchmark suite | Current: 8367c35 | Previous: 1feac18 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.028 s |
0.028 s |
1 |
private-kernel-reset |
0.16 s |
0.185 s |
0.86 |
private-kernel-tail |
0.011 s |
0.011 s |
1 |
rollup-base-private |
0.305 s |
0.304 s |
1.00 |
rollup-base-public |
0.192 s |
0.192 s |
1 |
rollup-block-root |
11.5 s |
12.1 s |
0.95 |
rollup-merge |
0.004 s |
0.004 s |
1 |
rollup-root |
0.013 s |
0.013 s |
1 |
semaphore-depth-10 |
0.02 s |
0.02 s |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Memory
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
private-kernel-inner |
316.51 MB |
316.51 MB |
1 |
private-kernel-reset |
572.22 MB |
572.31 MB |
1.00 |
private-kernel-tail |
231.44 MB |
231.43 MB |
1.00 |
rollup-base-private |
1440 MB |
1440 MB |
1 |
rollup-base-public |
1470 MB |
1470 MB |
1 |
rollup-block-root-empty |
400.74 MB |
402.96 MB |
0.99 |
rollup-block-root-single-tx |
7220 MB |
7220 MB |
1 |
rollup-block-root |
7220 MB |
7220 MB |
1 |
rollup-merge |
385.03 MB |
385.03 MB |
1 |
rollup-root |
446.38 MB |
445.96 MB |
1.00 |
semaphore_depth_10 |
128.47 MB |
128.47 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Memory
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
private-kernel-inner |
241.29 MB |
241.29 MB |
1 |
private-kernel-reset |
263.55 MB |
263.55 MB |
1 |
private-kernel-tail |
216.78 MB |
216.78 MB |
1 |
rollup-base-private |
549.27 MB |
549.27 MB |
1 |
rollup-base-public |
541.72 MB |
541.72 MB |
1 |
rollup-block-root |
1450 MB |
1450 MB |
1 |
rollup-merge |
370.98 MB |
370.98 MB |
1 |
rollup-root |
376.99 MB |
376.91 MB |
1.00 |
semaphore_depth_10 |
93.04 MB |
93.04 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Test Suite Duration
Details
| Benchmark suite | Current: 8071d98 | Previous: 1feac18 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
66 s |
65 s |
1.02 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts |
99 s |
100 s |
0.99 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
44 s |
48 s |
0.92 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
194 s |
192 s |
1.01 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
183 s |
182 s |
1.01 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
61 s |
60 s |
1.02 |
test_report_noir-lang_noir-bignum_ |
353 s |
375 s |
0.94 |
test_report_noir-lang_noir_bigcurve_ |
225 s |
227 s |
0.99 |
test_report_noir-lang_sha512_ |
30 s |
30 s |
1 |
test_report_zkpassport_noir_rsa_ |
23 s |
23 s |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
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: 5f4cd2d | Previous: e2a52b7 | Ratio |
|---|---|---|---|
test_report_noir-lang_noir_bigcurve_ |
284 s |
230 s |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
So we have this size regression:
And this execution trace regression:
We have this final SSA on master: And this SSA on this PR: The second call to f1 is with a constant input. This gets simplified to a single make array. As Due to the other benefits in this PR I think it is ok to eat this edge case. Especially as it is only occurring for |
compiler/noirc_evaluator/src/ssa/opt/inline_functions_with_at_most_one_instruction.rs
Show resolved
Hide resolved
TomAFrench
left a comment
There was a problem hiding this comment.
LGTM, will defer to @michaeljklein or @aakoshh when it comes to hitting merge.
aakoshh
left a comment
There was a problem hiding this comment.
I have no problem with merging stuff into the audited code, I'm just writing a post-audit report at this point, but I think some extra comments and magic-number outsourcing could be helpful to explain in the code why we went from 1 to 10 in particular.
compiler/noirc_evaluator/src/ssa/opt/inline_simple_functions.rs
Outdated
Show resolved
Hide resolved
compiler/noirc_evaluator/src/ssa/opt/inline_simple_functions.rs
Outdated
Show resolved
Hide resolved
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(licm): Account for nested loops being control dependent when analyzing outer loops (noir-lang/noir#8593) chore(refactor): Switch unreachable function removal to use centralized call graph (noir-lang/noir#8578) chore(test): Allow lambdas in fuzzing (noir-lang/noir#8584) chore: use insta for execution_success stdout (noir-lang/noir#8576) chore: use generator instead of zero for ec-add predicate (noir-lang/noir#8552) fix: use predicate expression as binary result (noir-lang/noir#8583) fix(ssa): Do not generate apply functions when no lambda variants exist (noir-lang/noir#8573) chore: put `nargo expand` snapshosts in the same directory (noir-lang/noir#8577) chore: Use FxHashMap for TypeBindings (noir-lang/noir#8574) chore(experimental): use larger stack size for parsing (noir-lang/noir#8347) chore: use insta snapshots for compile_failure stderr (noir-lang/noir#8569) chore(inlining): Mark functions with <= 10 instructions and no control flow as inline always (noir-lang/noir#8533) chore(ssa): Add weighted edges to call graph, move callers and callees methods to call graph (noir-lang/noir#8513) fix(frontend): Override to allow empty array input (noir-lang/noir#8568) fix: avoid logging all unused params in DIE pass (noir-lang/noir#8566) chore: bump external pinned commits (noir-lang/noir#8562) chore(deps): bump base-x from 3.0.9 to 3.0.11 (noir-lang/noir#8555) chore(fuzz): Call function pointers (noir-lang/noir#8531) feat: C++ codegen for msgpack (noir-lang/noir#7716) feat(performance): brillig array set optimization (noir-lang/noir#8550) chore(fuzz): AST fuzzer to use function valued arguments (Part 1) (noir-lang/noir#8514) fix(licm): Check whether the loop is executed when hoisting with a predicate (noir-lang/noir#8546) feat: Implement $crate (noir-lang/noir#8537) fix: add offset to ArrayGet (noir-lang/noir#8536) chore: remove some unused enum variants and functions (noir-lang/noir#8538) fix: disallow `()` in entry points (noir-lang/noir#8529) chore: Remove println in ssa interpreter (noir-lang/noir#8528) fix: don't overflow when casting signed value to u128 (noir-lang/noir#8526) chore(performance): Enable hoisting pure with predicate calls (noir-lang/noir#8522) feat(fuzz): AST fuzzing with SSA interpreter (noir-lang/noir#8436) chore: Add u1 ops to interpreter, convert Value panics to errors (noir-lang/noir#8469) chore: Release Noir(1.0.0-beta.6) (noir-lang/noir#8438) chore(fuzz): AST generator to add `ctx_limit` to all functions (noir-lang/noir#8507) fix(inlining): Use centralized CallGraph structure for inline info computation (noir-lang/noir#8489) fix: remove private builtins from `Field` impl (noir-lang/noir#8496) feat: primitive types are no longer keywords (noir-lang/noir#8470) fix: parenthesized pattern, and correct 1-element tuple printing (noir-lang/noir#8482) fix: fix visibility of methods in `std::meta` (noir-lang/noir#8497) fix: Change `can_be_main` to be recursive (noir-lang/noir#8501) chore: add SSA interpreter test for higher order functions (noir-lang/noir#8486) fix(frontend)!: Ban zero sized arrays and strings as program input (noir-lang/noir#8491) fix!: remove `to_be_radix` and `to_le_radix` from stdlib interface (noir-lang/noir#8495) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(licm): Account for nested loops being control dependent when analyzing outer loops (noir-lang/noir#8593) chore(refactor): Switch unreachable function removal to use centralized call graph (noir-lang/noir#8578) chore(test): Allow lambdas in fuzzing (noir-lang/noir#8584) chore: use insta for execution_success stdout (noir-lang/noir#8576) chore: use generator instead of zero for ec-add predicate (noir-lang/noir#8552) fix: use predicate expression as binary result (noir-lang/noir#8583) fix(ssa): Do not generate apply functions when no lambda variants exist (noir-lang/noir#8573) chore: put `nargo expand` snapshosts in the same directory (noir-lang/noir#8577) chore: Use FxHashMap for TypeBindings (noir-lang/noir#8574) chore(experimental): use larger stack size for parsing (noir-lang/noir#8347) chore: use insta snapshots for compile_failure stderr (noir-lang/noir#8569) chore(inlining): Mark functions with <= 10 instructions and no control flow as inline always (noir-lang/noir#8533) chore(ssa): Add weighted edges to call graph, move callers and callees methods to call graph (noir-lang/noir#8513) fix(frontend): Override to allow empty array input (noir-lang/noir#8568) fix: avoid logging all unused params in DIE pass (noir-lang/noir#8566) chore: bump external pinned commits (noir-lang/noir#8562) chore(deps): bump base-x from 3.0.9 to 3.0.11 (noir-lang/noir#8555) chore(fuzz): Call function pointers (noir-lang/noir#8531) feat: C++ codegen for msgpack (noir-lang/noir#7716) feat(performance): brillig array set optimization (noir-lang/noir#8550) chore(fuzz): AST fuzzer to use function valued arguments (Part 1) (noir-lang/noir#8514) fix(licm): Check whether the loop is executed when hoisting with a predicate (noir-lang/noir#8546) feat: Implement $crate (noir-lang/noir#8537) fix: add offset to ArrayGet (noir-lang/noir#8536) chore: remove some unused enum variants and functions (noir-lang/noir#8538) fix: disallow `()` in entry points (noir-lang/noir#8529) chore: Remove println in ssa interpreter (noir-lang/noir#8528) fix: don't overflow when casting signed value to u128 (noir-lang/noir#8526) chore(performance): Enable hoisting pure with predicate calls (noir-lang/noir#8522) feat(fuzz): AST fuzzing with SSA interpreter (noir-lang/noir#8436) chore: Add u1 ops to interpreter, convert Value panics to errors (noir-lang/noir#8469) chore: Release Noir(1.0.0-beta.6) (noir-lang/noir#8438) chore(fuzz): AST generator to add `ctx_limit` to all functions (noir-lang/noir#8507) fix(inlining): Use centralized CallGraph structure for inline info computation (noir-lang/noir#8489) fix: remove private builtins from `Field` impl (noir-lang/noir#8496) feat: primitive types are no longer keywords (noir-lang/noir#8470) fix: parenthesized pattern, and correct 1-element tuple printing (noir-lang/noir#8482) fix: fix visibility of methods in `std::meta` (noir-lang/noir#8497) fix: Change `can_be_main` to be recursive (noir-lang/noir#8501) chore: add SSA interpreter test for higher order functions (noir-lang/noir#8486) fix(frontend)!: Ban zero sized arrays and strings as program input (noir-lang/noir#8491) fix!: remove `to_be_radix` and `to_le_radix` from stdlib interface (noir-lang/noir#8495) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(licm): Account for nested loops being control dependent when analyzing outer loops (noir-lang/noir#8593) chore(refactor): Switch unreachable function removal to use centralized call graph (noir-lang/noir#8578) chore(test): Allow lambdas in fuzzing (noir-lang/noir#8584) chore: use insta for execution_success stdout (noir-lang/noir#8576) chore: use generator instead of zero for ec-add predicate (noir-lang/noir#8552) fix: use predicate expression as binary result (noir-lang/noir#8583) fix(ssa): Do not generate apply functions when no lambda variants exist (noir-lang/noir#8573) chore: put `nargo expand` snapshosts in the same directory (noir-lang/noir#8577) chore: Use FxHashMap for TypeBindings (noir-lang/noir#8574) chore(experimental): use larger stack size for parsing (noir-lang/noir#8347) chore: use insta snapshots for compile_failure stderr (noir-lang/noir#8569) chore(inlining): Mark functions with <= 10 instructions and no control flow as inline always (noir-lang/noir#8533) chore(ssa): Add weighted edges to call graph, move callers and callees methods to call graph (noir-lang/noir#8513) fix(frontend): Override to allow empty array input (noir-lang/noir#8568) fix: avoid logging all unused params in DIE pass (noir-lang/noir#8566) chore: bump external pinned commits (noir-lang/noir#8562) chore(deps): bump base-x from 3.0.9 to 3.0.11 (noir-lang/noir#8555) chore(fuzz): Call function pointers (noir-lang/noir#8531) feat: C++ codegen for msgpack (noir-lang/noir#7716) feat(performance): brillig array set optimization (noir-lang/noir#8550) chore(fuzz): AST fuzzer to use function valued arguments (Part 1) (noir-lang/noir#8514) fix(licm): Check whether the loop is executed when hoisting with a predicate (noir-lang/noir#8546) feat: Implement $crate (noir-lang/noir#8537) fix: add offset to ArrayGet (noir-lang/noir#8536) chore: remove some unused enum variants and functions (noir-lang/noir#8538) fix: disallow `()` in entry points (noir-lang/noir#8529) chore: Remove println in ssa interpreter (noir-lang/noir#8528) fix: don't overflow when casting signed value to u128 (noir-lang/noir#8526) chore(performance): Enable hoisting pure with predicate calls (noir-lang/noir#8522) feat(fuzz): AST fuzzing with SSA interpreter (noir-lang/noir#8436) chore: Add u1 ops to interpreter, convert Value panics to errors (noir-lang/noir#8469) chore: Release Noir(1.0.0-beta.6) (noir-lang/noir#8438) chore(fuzz): AST generator to add `ctx_limit` to all functions (noir-lang/noir#8507) fix(inlining): Use centralized CallGraph structure for inline info computation (noir-lang/noir#8489) fix: remove private builtins from `Field` impl (noir-lang/noir#8496) feat: primitive types are no longer keywords (noir-lang/noir#8470) fix: parenthesized pattern, and correct 1-element tuple printing (noir-lang/noir#8482) fix: fix visibility of methods in `std::meta` (noir-lang/noir#8497) fix: Change `can_be_main` to be recursive (noir-lang/noir#8501) chore: add SSA interpreter test for higher order functions (noir-lang/noir#8486) fix(frontend)!: Ban zero sized arrays and strings as program input (noir-lang/noir#8491) fix!: remove `to_be_radix` and `to_le_radix` from stdlib interface (noir-lang/noir#8495) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Description
Problem*
Experimenting with resolving #8457
Summary*
We have a pass to inline functions with a single instruction (or none). In general, small functions without any control flow should be inlined as they should almost always lead to further optimizations.
This PR changes
inline_functions_with_at_most_one_instructiontoinline_simple_functions.As per the doc comments in this PR a simple function is defined as the following:
Additional Context
See #8533 (comment) to understand the regressions for
to_bytes_consistent_inliner_min.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.