Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Separate unconstrained functions during monomorphization #6894

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 20, 2024

Description

Problem*

Resolves #6841

Summary*

Adds a boolean is_unconstrained as an extra key during monomorphization. This should separate constrained from unconstrained functions. This key is infectious so every function called from an unconstrained function will also be unconstrained.

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 bot commented Dec 20, 2024

Changes to Brillig bytecode sizes

Generated at commit: be2e56cb2e0d9df8347084533b67ee23ba330364, compared to commit: b23e85bfbff4192d228bcaae4421d6cdfa5614f2

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
to_bytes_integration +489 ❌ +555.68%
inline_decompose_hint_brillig_call +47 ❌ +235.00%
array_len +176 ❌ +183.33%
to_bytes_consistent +207 ❌ +153.33%
to_le_bytes +167 ❌ +136.89%

Full diff report 👇
Program Brillig opcodes (+/-) %
to_bytes_integration 577 (+489) +555.68%
inline_decompose_hint_brillig_call 67 (+47) +235.00%
array_len 272 (+176) +183.33%
to_bytes_consistent 342 (+207) +153.33%
to_le_bytes 289 (+167) +136.89%
hash_to_field 309 (+170) +122.30%
6 2,642 (+1,451) +121.83%
conditional_regression_short_circuit 2,715 (+1,451) +114.79%
sha256 5,095 (+2,687) +111.59%
regression_4449 1,546 (+795) +105.86%
sha256_var_witness_const_regression 2,661 (+1,353) +103.44%
sha256_var_size_regression 3,349 (+1,623) +94.03%
array_sort 569 (+273) +92.23%
array_dynamic_nested_blackbox_input 1,688 (+787) +87.35%
ecdsa_secp256k1 1,690 (+782) +86.12%
to_be_bytes 369 (+157) +74.06%
ram_blowup_regression 1,637 (+668) +68.94%
sha256_var_padding_regression 8,622 (+3,493) +68.10%
conditional_1 1,972 (+784) +65.99%
sha256_regression 11,170 (+4,250) +61.42%
array_dynamic_blackbox_input 1,652 (+620) +60.08%
sha2_byte 3,736 (+961) +34.63%
hashmap 26,656 (+4,843) +22.20%
u128 2,962 (+205) +7.44%
slices 2,340 (+154) +7.04%
merkle_insert 805 (+40) +5.23%
bigint 2,251 (+56) +2.55%
regression_4709 134,008 (+271) +0.20%
regression_5252 4,601 (-3) -0.07%
simple_shield 905 (-3) -0.33%
brillig_cow_regression 2,159 (-8) -0.37%
sha256_brillig_performance_regression 1,654 (-8) -0.48%
keccak256 2,159 (-38) -1.73%
reference_counts 302 (-171) -36.15%

Copy link
Contributor

github-actions bot commented Dec 20, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.48M
workspace 123.77M
regression_4709 422.91M
ram_blowup_regression 1.58G
private-kernel-tail 201.65M
private-kernel-reset 716.47M
private-kernel-inner 291.89M
parity-root 171.94M

Copy link
Contributor

github-actions bot commented Dec 20, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 1.451s 1%
regression_4709 0.826s 1%
ram_blowup_regression 16.178s 2%
rollup-base-public 115.324s 1%
rollup-base-private 95.750s 0%
private-kernel-tail 1.027s -1%
private-kernel-reset 7.469s 4%
private-kernel-inner 2.136s 1%
parity-root 0.724s -2%
noir-contracts 87.402s 1%

Copy link
Contributor

github-actions bot commented Dec 20, 2024

Execution Sample

Program Execution Time %
sha256_regression 0.632s 0%
regression_4709 0.404s 1%
ram_blowup_regression 4.444s -3%
rollup-base-public 20.951s -4%
rollup-base-private 19.135s -4%
private-kernel-tail 0.696s -2%
private-kernel-reset 1.496s 0%
private-kernel-inner 0.959s -2%
parity-root 0.519s -3%

@@ -276,19 +271,12 @@ pub fn create_program(
(generated_acirs, generated_brillig, brillig_function_names, error_types),
ssa_level_warnings,
) = optimize_into_acir(program, options)?;
if options.force_brillig_output {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do think this assert is correct and should remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather remove the force_brillig option from SSA entirely since otherwise it'd only be used for this one assert

Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove the option but case on whether we have a single main function that is marked unconstrained

Copy link
Contributor

Choose a reason for hiding this comment

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

As otherwise we are going to error out when asserting the number of acir artifacts against the number of function signatures.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also just update how we collect function sigs

if f.inline_type.is_entry_point() || f.id == Program::main_id() {

is_entry_point is going to return true for functions marked with #[fold]. However, if we are force compiling to Brillig we do not need to collect this signature. The func sigs are only used for constructing acir artifacts. This feels like the best solution here.

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.

Separate runtimes before SSA
2 participants