feat(ssa): Bring back tracking of RC instructions during DIE

[vezenovm]

Description

Problem*

Brings back the optimization with a fix that was removed for correctness

Summary*

This came to mind while working on #6782 as actually being a very beneficial optimization. After #6782 we will have multiple locations (loop invariant code motion and constant folding) where upon moving a make_array instructions, we will conservatively issue inc_rc instructions in case that array is mutable. However, if the array is never mutated or mutably borrowed that will lead to us unnecessary incrementing the reference counter.

Compared to the original RcTracker:

• We now check ref counts based upon the type of an array per block
• We check whether on an array type has been mutated or passed into a call. Calls can accept mutable arguments as inputs and contain calls themselves. If the mutation instruction is simplified after inlining we can potentially remove a necessary inc rc

This lets us get rid of most increment rcs on arrays that are never mutated.

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.

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 2188c7254f25d6145c4b7066124bcb2fc6d495fd, compared to commit: b88db67a4fa92f861329105fb732a7b1309620fe

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_struct_array_conditional	-57 ✅	-9.00%
slices	-228 ✅	-9.44%
reference_counts	-63 ✅	-11.75%
nested_array_dynamic	-330 ✅	-11.84%
nested_array_in_slice	-213 ✅	-14.74%
slice_regex	-396 ✅	-15.06%
wildcard_type	-57 ✅	-16.06%
nested_array_dynamic_simple	-33 ✅	-24.09%
fold_complex_outputs	-159 ✅	-24.84%
brillig_nested_arrays	-225 ✅	-55.28%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_4709	133,737 (-777)	-0.58%
debug_logs	5,148 (-36)	-0.69%
slice_dynamic_index	2,507 (-18)	-0.71%
pedersen_hash	373 (-3)	-0.80%
bench_2_to_17	328 (-3)	-0.91%
array_dynamic	319 (-3)	-0.93%
modulus	1,762 (-18)	-1.01%
check_large_field_bits	292 (-3)	-1.02%
to_be_bytes	212 (-3)	-1.40%
pedersen_commitment	209 (-3)	-1.42%
fold_2_to_17	584 (-9)	-1.52%
fold_numeric_generic_poseidon	763 (-12)	-1.55%
no_predicates_numeric_generic_poseidon	763 (-12)	-1.55%
regression	945 (-15)	-1.56%
brillig_oracle	370 (-6)	-1.60%
hashmap	21,801 (-411)	-1.85%
poseidon_bn254_hash_width_3	5,425 (-108)	-1.95%
poseidon_bn254_hash	5,425 (-108)	-1.95%
7_function	579 (-12)	-2.03%
7	144 (-3)	-2.04%
blake3	144 (-3)	-2.04%
hash_to_field	139 (-3)	-2.11%
aes128_encrypt	552 (-12)	-2.13%
keccak256	2,197 (-48)	-2.14%
loop_invariant_regression	137 (-3)	-2.14%
u128	2,757 (-63)	-2.23%
6_array	392 (-9)	-2.24%
conditional_regression_661	127 (-3)	-2.31%
sha2_byte	2,767 (-66)	-2.33%
poseidonsponge_x5_254	4,251 (-102)	-2.34%
conditional_2	116 (-3)	-2.52%
hint_black_box	336 (-9)	-2.61%
conditional_1	1,188 (-33)	-2.70%
sha256_regression	6,917 (-195)	-2.74%
strings	917 (-27)	-2.86%
if_else_chain	101 (-3)	-2.88%
brillig_cow_regression	2,167 (-69)	-3.09%
regression_5252	4,594 (-147)	-3.10%
uhashmap	14,004 (-450)	-3.11%
simple_shield	908 (-30)	-3.20%
ecdsa_secp256k1	908 (-30)	-3.20%
ram_blowup_regression	969 (-33)	-3.29%
array_dedup_regression	259 (-9)	-3.36%
nested_dyn_array_regression_5782	172 (-6)	-3.37%
conditional_regression_421	83 (-3)	-3.49%
sha256_var_padding_regression	5,129 (-189)	-3.55%
brillig_pedersen	565 (-21)	-3.58%
pedersen_check	565 (-21)	-3.58%
brillig_cow	372 (-15)	-3.88%
simple_radix	74 (-3)	-3.90%
sha256_var_witness_const_regression	1,308 (-54)	-3.96%
array_dynamic_nested_blackbox_input	901 (-39)	-4.15%
array_dynamic_blackbox_input	1,032 (-45)	-4.18%
higher_order_functions	677 (-30)	-4.24%
slice_loop	269 (-12)	-4.27%
bigint	2,195 (-102)	-4.44%
sha256_brillig_performance_regression	1,662 (-78)	-4.48%
sha256	2,408 (-120)	-4.75%
conditional_regression_short_circuit	1,264 (-63)	-4.75%
array_to_slice	718 (-36)	-4.77%
6	1,191 (-60)	-4.80%
merkle_insert	765 (-39)	-4.85%
global_consts	227 (-12)	-5.02%
brillig_uninitialized_arrays	51 (-3)	-5.56%
to_bits	201 (-12)	-5.63%
regression_4449	751 (-45)	-5.65%
regression_6674_1	247 (-15)	-5.73%
to_bytes_integration	88 (-6)	-6.38%
regression_5045	87 (-6)	-6.45%
side_effects_constrain_array	129 (-9)	-6.52%
brillig_constant_reference_regression	84 (-6)	-6.67%
regression_6674_2	250 (-18)	-6.72%
databus_composite_calldata	390 (-30)	-7.14%
sha256_var_size_regression	1,726 (-138)	-7.40%
struct_inputs	258 (-21)	-7.53%
simple_2d_array	106 (-9)	-7.83%
brillig_rc_regression_6123	170 (-15)	-8.11%
to_bytes_consistent	135 (-12)	-8.16%
regression_6674_3	555 (-54)	-8.87%
to_le_bytes	122 (-12)	-8.96%
regression_struct_array_conditional	576 (-57)	-9.00%
slices	2,186 (-228)	-9.44%
reference_counts	473 (-63)	-11.75%
nested_array_dynamic	2,457 (-330)	-11.84%
nested_array_in_slice	1,232 (-213)	-14.74%
slice_regex	2,233 (-396)	-15.06%
wildcard_type	298 (-57)	-16.06%
nested_array_dynamic_simple	104 (-33)	-24.09%
fold_complex_outputs	481 (-159)	-24.84%
brillig_nested_arrays	182 (-225)	-55.28%

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: 2188c7254f25d6145c4b7066124bcb2fc6d495fd, compared to commit: b88db67a4fa92f861329105fb732a7b1309620fe

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
nested_array_dynamic	-309 ✅	-8.19%
side_effects_constrain_array	-9 ✅	-9.47%
slice_regex	-387 ✅	-10.02%
nested_array_in_slice	-201 ✅	-11.39%
wildcard_type	-75 ✅	-12.65%
reference_counts	-60 ✅	-12.96%
fold_complex_outputs	-156 ✅	-16.88%
nested_array_dynamic_simple	-33 ✅	-26.83%
brillig_nested_arrays	-225 ✅	-59.21%
regression_4709	-51,320,073 ✅	-97.72%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
bench_2_to_17	589,841 (-3)	-0.00%
fold_2_to_17	1,093,925 (-9)	-0.00%
modulus	19,172 (-18)	-0.09%
check_large_field_bits	2,740 (-3)	-0.11%
to_be_bytes	2,452 (-3)	-0.12%
brillig_oracle	2,751 (-6)	-0.22%
u128	24,962 (-63)	-0.25%
aes128_encrypt	4,719 (-12)	-0.25%
loop_invariant_regression	1,164 (-3)	-0.26%
7	1,106 (-3)	-0.27%
blake3	1,106 (-3)	-0.27%
to_bytes_integration	2,065 (-6)	-0.29%
hash_to_field	908 (-3)	-0.33%
keccak256	34,777 (-120)	-0.34%
ram_blowup_regression	778,650 (-2,748)	-0.35%
brillig_cow_regression	519,086 (-1,851)	-0.36%
sha256_var_padding_regression	222,216 (-801)	-0.36%
slice_dynamic_index	4,667 (-18)	-0.38%
pedersen_hash	669 (-3)	-0.45%
poseidonsponge_x5_254	183,753 (-831)	-0.45%
regression_5252	915,026 (-4,185)	-0.46%
7_function	2,537 (-12)	-0.47%
poseidon_bn254_hash	162,594 (-822)	-0.50%
poseidon_bn254_hash_width_3	162,594 (-822)	-0.50%
to_le_bytes	1,152 (-6)	-0.52%
uhashmap	145,549 (-771)	-0.53%
regression	2,816 (-15)	-0.53%
sha2_byte	47,309 (-266)	-0.56%
array_dynamic	498 (-3)	-0.60%
sha256_brillig_performance_regression	23,194 (-147)	-0.63%
global_consts	1,773 (-12)	-0.67%
debug_logs	5,160 (-36)	-0.69%
sha256_regression	118,707 (-916)	-0.77%
hashmap	54,143 (-504)	-0.92%
pedersen_commitment	293 (-3)	-1.01%
hint_black_box	717 (-9)	-1.24%
6_array	1,631 (-21)	-1.27%
brillig_cow	1,140 (-15)	-1.30%
fold_numeric_generic_poseidon	5,143 (-75)	-1.44%
no_predicates_numeric_generic_poseidon	5,143 (-75)	-1.44%
strings	1,770 (-27)	-1.50%
to_bytes_consistent	754 (-12)	-1.57%
regression_6674_1	819 (-15)	-1.80%
simple_shield	2,871 (-54)	-1.85%
slice_loop	942 (-18)	-1.88%
to_bits	614 (-12)	-1.92%
array_to_slice	1,670 (-36)	-2.11%
brillig_pedersen	972 (-21)	-2.11%
pedersen_check	972 (-21)	-2.11%
sha256_var_size_regression	16,377 (-356)	-2.13%
regression_6674_2	822 (-18)	-2.14%
merkle_insert	3,769 (-87)	-2.26%
array_dynamic_blackbox_input	18,209 (-427)	-2.29%
higher_order_functions	1,255 (-30)	-2.33%
conditional_regression_661	120 (-3)	-2.44%
conditional_2	119 (-3)	-2.46%
ecdsa_secp256k1	6,789 (-200)	-2.86%
regression_struct_array_conditional	1,679 (-51)	-2.95%
if_else_chain	98 (-3)	-2.97%
array_dedup_regression	677 (-21)	-3.01%
regression_6674_3	1,615 (-54)	-3.24%
conditional_regression_421	89 (-3)	-3.26%
nested_dyn_array_regression_5782	171 (-6)	-3.39%
conditional_1	5,717 (-203)	-3.43%
sha256_var_witness_const_regression	7,200 (-266)	-3.56%
struct_inputs	566 (-21)	-3.58%
simple_radix	69 (-3)	-4.17%
sha256	14,996 (-672)	-4.29%
array_dynamic_nested_blackbox_input	4,550 (-209)	-4.39%
databus_composite_calldata	617 (-30)	-4.64%
slices	3,365 (-171)	-4.84%
brillig_rc_regression_6123	290 (-15)	-4.92%
conditional_regression_short_circuit	7,528 (-400)	-5.05%
6	7,450 (-400)	-5.10%
brillig_uninitialized_arrays	44 (-3)	-6.38%
simple_2d_array	125 (-9)	-6.72%
regression_4449	200,857 (-14,843)	-6.88%
brillig_constant_reference_regression	81 (-6)	-6.90%
nested_array_dynamic	3,464 (-309)	-8.19%
side_effects_constrain_array	86 (-9)	-9.47%
slice_regex	3,474 (-387)	-10.02%
nested_array_in_slice	1,564 (-201)	-11.39%
wildcard_type	518 (-75)	-12.65%
reference_counts	403 (-60)	-12.96%
fold_complex_outputs	768 (-156)	-16.88%
nested_array_dynamic_simple	90 (-33)	-26.83%
brillig_nested_arrays	155 (-225)	-59.21%
regression_4709	1,196,640 (-51,320,073)	-97.72%

[github-actions]

Peak Memory Sample

Program	Peak Memory	%
keccak256	79.14M	0%
workspace	121.99M	-1%
regression_4709	295.99M	0%
ram_blowup_regression	2.44G	0%
private-kernel-tail	210.43M	0%
private-kernel-reset	862.08M	0%
private-kernel-inner	307.78M	0%
parity-root	175.78M	0%

[github-actions]

Compilation Sample

Program	Compilation Time	%
sha256_regression	0m1.463s	-14%
regression_4709	0m0.736s	-9%
ram_blowup_regression	0m17.032s	-1%
private-kernel-tail	0m1.243s	-19%
private-kernel-reset	0m9.323s	6%
private-kernel-inner	0m2.747s	1%
parity-root	0m1.203s	28%
noir-contracts	2m52.240s	1%

[github-actions]

Changes to circuit sizes

Generated at commit: 2188c7254f25d6145c4b7066124bcb2fc6d495fd, compared to commit: b88db67a4fa92f861329105fb732a7b1309620fe

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
regression_struct_array_conditional	-16 ✅	-18.39%	-24 ✅	-0.74%
nested_array_in_slice	-70 ✅	-6.48%	-151 ✅	-2.65%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
array_dynamic_nested_blackbox_input	253 (-12)	-4.53%	7,326 (-35)	-0.48%
regression_struct_array_conditional	71 (-16)	-18.39%	3,203 (-24)	-0.74%
nested_array_in_slice	1,010 (-70)	-6.48%	5,550 (-151)	-2.65%

[jfecher]

Maybe it'd be easier if we promote this to its own pass rather than basing it off of DIE logic

[vezenovm]

Maybe it'd be easier if we promote this to its own pass rather than basing it off of DIE logic

Perhaps, I was thinking the same thing as it makes DIE more complex. On the flip side, I wanted to avoid another pass for this logic which can be contained to DIE and in a way does remove useless instructions (even if they are not "dead").

[jfecher]

I'd like to see if this breaks an external repos if we change rcs to start at 1 again instead of 2. No errors in cargo test locally at least, but I'm checking in #6797 in the meantime.

[vezenovm]

regression_4709 -51,320,073 ✅ -97.72%

This PR is where we get the largest benefit for patterns like this test where we are simply reading from a global array in a loop.

[jfecher]

My main hesitancy with this PR is that it was removed for correctness before and is re-added now but what changed? As far as I can tell it's identical. Won't we run into sync issues again? Or run into them once #6797 is added as well?

[vezenovm]

My main hesitancy with this PR is that it was removed for correctness before and is re-added now but what changed? As far as I can tell it's identical. Won't we run into sync issues again? Or run into them once #6797 is added as well?

This PR also adds a check for whether an array type was passed to a call in the block (previously we only checked for stores). I have also tested it here (AztecProtocol/aztec-packages#10399) which passes all noir-projects tests in atec-packages (the yarn-project-test failures look to be from the p2p network tests).

It does look this PR would run into issues with #6797. But don't we have these issues already if we were to add back #6797?

[jfecher]

Hmm alright, I'm okay with merging this then. Are you confident passing arrays into calls was the original issue or is this something we should continue investigating?

It does look this PR would run into issues with #6797. But don't we have these issues already if we were to add back #6797?

I'm not concerned with #6797, the only failure there was from the failure that was originally specific to that rc change rather than the RcTracker changes here.

[vezenovm]

Hmm alright, I'm okay with merging this then. Are you confident passing arrays into calls was the original issue or is this something we should continue investigating?

Working with the original RC tracker (no Call checks), I had not finished reproducing the error from the noir protocol circuits failure yet. I then decided to try things against the reference_counts test with different inlining settings. We were hitting the case where copy_mut was failing as array was not being incremented as expected. This made sense to me as I was already thinking we need to treat Call the same way we treat Store for checking non mutated arrays. The failure from reference_counts is the one laid out in the do_not_remove_inc_rc_if_used_as_call_arg unit test (one of the only other new things in this PR).

I can continue trying to reproduce the exact Noir snippet from aztec-packages, but I do think not checking arrays passed to calls was the culprit of the broken RC tracker.

[jfecher]

That sounds good, didn't know you were able to reproduce an issue using reference_counts