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: Add Instruction::MakeArray to SSA #6071

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Sep 17, 2024

Description

Problem*

Resolves #1733

Summary*

Adds a MakeArray instruction to explicitly create a new array. Now no other ValueIds can contain other ValueIds which simplifies a couple passes.

Additional Context

Still working on getting this working in brillig.

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.

@TomAFrench
Copy link
Member

Seems like --show-ssa fixes the compilation failures on the few programs I tried.

@TomAFrench
Copy link
Member

Disabling the normalization of valueids shows that we're deleting instructions in the DIE pass which shouldn't be removed. For example the load instruction when returning arr in quicksort is removed from array_sort.

This is happening because somehow the return value of this function (the loaded value) becomes a Value::Function rather than a Value::Instruction. We only mark return values as used if they're constants, instructions or block params so we end up removing the loaded instruction.

@TomAFrench
Copy link
Member

I've merged in master so that this branch is up to date so will be handing this PR back now.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Changes to Brillig bytecode sizes

Generated at commit: 42c75cca1668f0fda203ffe881a448a4b6615792, compared to commit: 7cc7197bf7b2e41c07e8d1979f7e9d45c676d11b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
slice_regex +2,923 ❌ +119.84%
eddsa +5,500 ❌ +49.20%
brillig_slices +118 ❌ +25.60%
bigint +482 ❌ +23.63%
side_effects_constrain_array +24 ❌ +22.22%
slices +434 ❌ +21.42%
databus_two_calldata_simple +22 ❌ +20.75%

Full diff report 👇
Program Brillig opcodes (+/-) %
slice_regex 5,362 (+2,923) +119.84%
eddsa 16,679 (+5,500) +49.20%
brillig_slices 579 (+118) +25.60%
bigint 2,522 (+482) +23.63%
side_effects_constrain_array 132 (+24) +22.22%
slices 2,460 (+434) +21.42%
databus_two_calldata_simple 128 (+22) +20.75%
databus 100 (+15) +17.65%
databus_in_fn_with_empty_arr 49 (+7) +16.67%
hashmap 30,358 (+3,986) +15.11%
databus_composite_calldata 590 (+73) +14.12%
sha256_var_padding_regression 5,729 (+700) +13.92%
sha2_byte 3,563 (+414) +13.15%
6 1,931 (+181) +10.34%
brillig_keccak 1,890 (+177) +10.33%
keccak256 1,890 (+177) +10.33%
sha256 2,050 (+189) +10.16%
sha256_var_size_regression 2,095 (+189) +9.92%
databus_two_calldata 274 (+24) +9.60%
debug_logs 5,462 (+470) +9.42%
fold_complex_outputs 586 (+50) +9.33%
nested_array_in_slice 1,310 (+110) +9.17%
sha256_regression 7,498 (+595) +8.62%
modulus 1,880 (+148) +8.55%
merkle_insert 1,450 (+114) +8.53%
conditional_regression_short_circuit 1,433 (+112) +8.48%
uhashmap 25,497 (+1,982) +8.43%
fold_2_to_17 561 (+38) +7.27%
sha256_var_witness_const_regression 1,365 (+91) +7.14%
array_to_slice 971 (+64) +7.06%
7_function 601 (+34) +6.00%
schnorr 1,586 (+86) +5.73%
aes128_encrypt 539 (+28) +5.48%
struct_inputs 293 (+15) +5.40%
u128 3,273 (+166) +5.34%
regression_5252 5,418 (+272) +5.29%
array_dynamic_blackbox_input 1,206 (+56) +4.87%
poseidonsponge_x5_254 4,508 (+189) +4.38%
nested_array_dynamic 2,401 (+99) +4.30%
regression_4449 829 (+28) +3.50%
pedersen_check 570 (+18) +3.26%
brillig_pedersen 570 (+18) +3.26%
fold_numeric_generic_poseidon 720 (+22) +3.15%
no_predicates_numeric_generic_poseidon 720 (+22) +3.15%
poseidon_bn254_hash_width_3 5,672 (+173) +3.15%
poseidon_bn254_hash 5,672 (+173) +3.15%
strings 925 (+27) +3.01%
brillig_cow_regression 2,392 (+56) +2.40%
brillig_sha256 756 (+14) +1.89%
array_dynamic_nested_blackbox_input 945 (+14) +1.50%
ecdsa_secp256k1 969 (+14) +1.47%
conditional_1 1,265 (+14) +1.12%

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Changes to circuit sizes

Generated at commit: 42c75cca1668f0fda203ffe881a448a4b6615792, compared to commit: 7cc7197bf7b2e41c07e8d1979f7e9d45c676d11b

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap -37,050 ✅ -39.68% -59,930 ✅ -39.78%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 56,333 (-37,050) -39.68% 90,731 (-59,930) -39.78%

@jfecher jfecher marked this pull request as ready for review October 9, 2024 11:43
@jfecher jfecher requested a review from a team October 9, 2024 11:43
@jfecher
Copy link
Contributor Author

jfecher commented Oct 9, 2024

This is passing all tests now and showing a surprisingly large improvement in the hashmap test but also large regressions in brillig bytecode sizes.

@TomAFrench
Copy link
Member

This is likely due to the fact that constant arrays are no long considered as "constants" by constant_allocation.rs. We're then not hoisting their usage up to avoid multiple initialisations.

@TomAFrench TomAFrench linked an issue Oct 9, 2024 that may be closed by this pull request
@jfecher
Copy link
Contributor Author

jfecher commented Oct 9, 2024

@TomAFrench hmm I tried updating is_constant_value there but doing so just leads to many panics on ValueIds not being available.

Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

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

LGTM

@jfecher
Copy link
Contributor Author

jfecher commented Oct 9, 2024

Going to hold off on merging this for a bit due to the brillig size regressions

@vezenovm
Copy link
Contributor

vezenovm commented Oct 9, 2024

Going to hold off on merging this for a bit due to the brillig size regressions

Yeah would be good to avoid this regression.

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.

keccakf1600 instructions aren't being deduplicated as expected Add MakeArray instruction to the ssa refactor
4 participants