fix: increment ref count when hoisting instructions returning array

[guipublic]

Description

Problem

Resolves #10988

Summary

Generalise ref count increase when hoisting MakeArray instructions to all instructions returning array.

Additional Context

User Documentation

Check one:

• No user documentation needed.
• Changes in docs/ included in this PR.
• [For Experimental Features] Changes in docs/ 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]

⚠️ 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: 7bc330a	Previous: 2c2a7b8	Ratio
test_report_zkpassport_noir-ecdsa_	2 s	1 s	2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: b1a01b0	Previous: 2c2a7b8	Ratio
rollup-tx-merge	0.003 s	0.002 s	1.50
sha512-100-bytes	0.104 s	0.083 s	1.25

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: a5b9fc1	Previous: 6979ab7	Ratio
perfectly_parallel_batch_inversion_opcodes	2791554 ns/iter (± 1832)	2261756 ns/iter (± 6237)	1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[vezenovm]

Looks good, just a couple nits regarding the tests and asserting invariants

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 471dea1	Previous: c9a8bf8	Ratio
sha512-100-bytes	2.053 s	1.561 s	1.32

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[aakoshh]

Have you looked at the increase in circuit size for global_var_regression_entry_points and global_var_regression_simple?

I had a quick look at global_var_regression_entry_points. It looks like the extra inc_rc instructions prevented the loop in check_acc_entry_point from being unrolled, which resulted in that block staying in place. Seems like bad luck.

The hoisting itself is the array_get from a global array, so perhaps it shouldn't happen, as we know global arrays are read-only, and while an inc_rc seems defensive, I think there already are other mechanisms to protect them. For example if we pass them to a function, they get cloned, or if we read a sub-array into a variable, it gets cloned, but in this case we just have EXPONENTIATE[i][j] in the loop and hoisted EXPONENTIATE[i] into the other loop, leaving an inc_rc on the sub-array in the loop. If we did this: let sub = EXPONENTIATE[i]; sub[j] then we would get a clone already, so I'm just thinking that maybe not everything that can be hoisted needs this new inc_rc being inserted?

[aakoshh]

To illustrate my point about the cloning already done by ownership, consider this code:

unconstrained fn main(i: u32, j: u32, k: u32) {
    let mut a = [[[1], [2]], [[3], [4]]];
    let _a_i = a[i];
    let _a_ij = a[i][j];
    let _a_ijk = a[i][j][k];
    a[i][j][k] = 5;
}

With --show-monomorphized we can see that there are .clone()s inserted:

• in RHS for the final sub-array that gets referred to by a new variable
• in the LHS for assignment

unconstrained fn main$f0(i$l0: u32, j$l1: u32, k$l2: u32) -> () {
    let mut a$l3 = [[[1], [2]], [[3], [4]]];
    let _a_i$l4 = a$l3[i$l0].clone();
    let _a_ij$l5 = a$l3[i$l0][j$l1].clone();
    let _a_ijk$l6 = a$l3[i$l0][j$l1][k$l2];
    a$l3[i$l0].clone()[j$l1].clone()[k$l2] = 5
}

This PR would insert further clones for the interim sub-arrays as well, which the ownership tried to avoid.

[github-actions]

ACVM Benchmarks

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
purely_sequential_opcodes	256639 ns/iter (± 631)	252275 ns/iter (± 1707)	1.02
perfectly_parallel_opcodes	219838 ns/iter (± 5459)	219900 ns/iter (± 6729)	1.00
perfectly_parallel_batch_inversion_opcodes	2782165 ns/iter (± 1128)	2781840 ns/iter (± 1474)	1.00

This comment was automatically generated by workflow using github-action-benchmark.

[guipublic]

Have you looked at the increase in circuit size for global_var_regression_entry_points and global_var_regression_simple?

I have investigated this and indeed it is due to the inc_rc for ArrayGet, as you said. In fact an ArrayGet that returns an array is returning a nested array from a parent array, and in that case the reference count is already taken care by the parent.
So I have taken a more conservative approach. The idea is to add the inc_rc only for new arrays, and this should only happen with Call, MakeArray and ArraySet.

[github-actions]

Test Suite Duration

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	147 s	151 s	0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	168 s	171 s	0.98
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	165 s	166 s	0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	379 s	370 s	1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	157 s	159 s	0.99
test_report_noir-lang_noir-bignum_	169 s	165 s	1.02
test_report_noir-lang_noir_bigcurve_	290 s	311 s	0.93
test_report_noir-lang_sha256_	18 s	17 s	1.06
test_report_noir-lang_sha512_	14 s	14 s	1
test_report_zkpassport_noir-ecdsa_	1 s	1 s	1
test_report_zkpassport_noir_rsa_	1 s	1 s	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Brillig Execution Time

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	0.04 s	0.041 s	0.98
private-kernel-reset	0.12 s	0.12 s	1
private-kernel-tail	0.007 s	0.007 s	1
rollup-block-root-first-empty-tx	0.004 s	0.004 s	1
rollup-block-root-single-tx	0.003 s	0.003 s	1
rollup-block-root	0.004 s	0.004 s	1
rollup-checkpoint-merge	0.002 s	0.002 s	1
rollup-root	0.01 s	0.012 s	0.83
rollup-tx-base-private	0.044 s	0.045 s	0.98
rollup-tx-base-public	0.057 s	0.058 s	0.98
rollup-tx-merge	0.002 s	0.002 s	1
semaphore-depth-10	0.033 s	0.027 s	1.22
sha512-100-bytes	0.021 s	0.021 s	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Compilation Time

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	2.362 s	2.33 s	1.01
private-kernel-reset	8.394 s	8.198 s	1.02
private-kernel-tail	2.616 s	2.482 s	1.05
rollup-block-root-first-empty-tx	1.426 s	1.438 s	0.99
rollup-block-root-single-tx	1.44 s	1.43 s	1.01
rollup-block-root	1.54 s	1.5 s	1.03
rollup-checkpoint-merge	1.556 s	1.492 s	1.04
rollup-checkpoint-root-single-block	403 s	392 s	1.03
rollup-checkpoint-root	397 s	408 s	0.97
rollup-root	3.312 s	4.998 s	0.66
rollup-tx-base-private	22.14 s	21.12 s	1.05
rollup-tx-base-public	89.82 s	84.4 s	1.06
rollup-tx-merge	1.496 s	1.366 s	1.10
semaphore-depth-10	0.927 s	0.971 s	0.95
sha512-100-bytes	1.521 s	1.596 s	0.95

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Brillig Artifact Size

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	671.1 KB	671.1 KB	1
private-kernel-reset	681.2 KB	681.2 KB	1
private-kernel-tail	291.7 KB	291.7 KB	1
rollup-block-root-first-empty-tx	245 KB	245 KB	1
rollup-block-root-single-tx	244.3 KB	244.3 KB	1
rollup-block-root	300.7 KB	300.7 KB	1
rollup-checkpoint-merge	275 KB	275 KB	1
rollup-checkpoint-root-single-block	528.7 KB	528.7 KB	1
rollup-checkpoint-root	567.9 KB	567.9 KB	1
rollup-root	463.2 KB	463.2 KB	1
rollup-tx-base-private	639.5 KB	639.5 KB	1
rollup-tx-base-public	756.7 KB	756.7 KB	1
rollup-tx-merge	190.5 KB	190.5 KB	1
semaphore-depth-10	2068.7 KB	2068.7 KB	1
sha512-100-bytes	162 KB	162 KB	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Artifact Size

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	939 KB	939 KB	1
private-kernel-reset	2064.4 KB	2064.4 KB	1
private-kernel-tail	469.1 KB	469.1 KB	1
rollup-block-root-first-empty-tx	220.5 KB	220.5 KB	1
rollup-block-root-single-tx	224 KB	224 KB	1
rollup-block-root	292.3 KB	292.3 KB	1
rollup-checkpoint-merge	385.9 KB	385.9 KB	1
rollup-checkpoint-root-single-block	48353.7 KB	48353.7 KB	1
rollup-checkpoint-root	48423 KB	48423 KB	1
rollup-root	1209 KB	1209 KB	1
rollup-tx-base-private	5444.7 KB	5444.7 KB	1
rollup-tx-base-public	4759.7 KB	4759.7 KB	1
rollup-tx-merge	168.8 KB	168.8 KB	1
semaphore-depth-10	551.2 KB	551.2 KB	1
sha512-100-bytes	474 KB	474 KB	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Brillig Compilation Time

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	1.204 s	1.202 s	1.00
private-kernel-reset	1.234 s	1.23 s	1.00
private-kernel-tail	1.11 s	1.042 s	1.07
rollup-block-root-first-empty-tx	1.366 s	1.412 s	0.97
rollup-block-root-single-tx	1.41 s	1.38 s	1.02
rollup-block-root	1.44 s	1.44 s	1
rollup-checkpoint-merge	1.406 s	1.38 s	1.02
rollup-checkpoint-root-single-block	1.88 s	1.9 s	0.99
rollup-checkpoint-root	2.01 s	1.92 s	1.05
rollup-root	1.46 s	2.484 s	0.59
rollup-tx-base-private	1.594 s	1.53 s	1.04
rollup-tx-base-public	1.602 s	1.626 s	0.99
rollup-tx-merge	1.41 s	1.318 s	1.07
semaphore-depth-10	0.246 s	0.248 s	0.99
sha512-100-bytes	0.195 s	0.204 s	0.96

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Execution Time

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	0.053 s	0.049 s	1.08
private-kernel-reset	0.241 s	0.253 s	0.95
private-kernel-tail	0.01 s	0.009 s	1.11
rollup-block-root-first-empty-tx	0.003 s	0.003 s	1
rollup-block-root-single-tx	0.002 s	0.003 s	0.67
rollup-block-root	0.004 s	0.004 s	1
rollup-checkpoint-merge	0.003 s	0.003 s	1
rollup-checkpoint-root-single-block	32.9 s	34 s	0.97
rollup-checkpoint-root	32.5 s	31.8 s	1.02
rollup-root	0.052 s	0.06 s	0.87
rollup-tx-base-private	0.335 s	0.337 s	0.99
rollup-tx-base-public	0.257 s	0.257 s	1
rollup-tx-merge	0.002 s	0.002 s	1
semaphore-depth-10	0.009 s	0.009 s	1
sha512-100-bytes	0.097 s	0.083 s	1.17

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Opcode count

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	20088 opcodes	20088 opcodes	1
private-kernel-reset	80496 opcodes	80496 opcodes	1
private-kernel-tail	8554 opcodes	8554 opcodes	1
rollup-block-root-first-empty-tx	1082 opcodes	1082 opcodes	1
rollup-block-root-single-tx	967 opcodes	967 opcodes	1
rollup-block-root	2169 opcodes	2169 opcodes	1
rollup-checkpoint-merge	1902 opcodes	1902 opcodes	1
rollup-checkpoint-root-single-block	1819274 opcodes	1819274 opcodes	1
rollup-checkpoint-root	1820467 opcodes	1820467 opcodes	1
rollup-root	49245 opcodes	49245 opcodes	1
rollup-tx-base-private	302291 opcodes	302291 opcodes	1
rollup-tx-base-public	257976 opcodes	257976 opcodes	1
rollup-tx-merge	1302 opcodes	1302 opcodes	1
semaphore-depth-10	5699 opcodes	5699 opcodes	1
sha512-100-bytes	13173 opcodes	13173 opcodes	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	285.96 MB	285.96 MB	1
private-kernel-reset	510.39 MB	510.39 MB	1
private-kernel-tail	258.99 MB	258.99 MB	1
rollup-block-root-first-empty-tx	338.67 MB	338.67 MB	1
rollup-block-root-single-tx	337.11 MB	337.11 MB	1
rollup-block-root	339.66 MB	339.66 MB	1
rollup-checkpoint-merge	339.99 MB	339.99 MB	1
rollup-checkpoint-root-single-block	11290 MB	11290 MB	1
rollup-checkpoint-root	11290 MB	11290 MB	1
rollup-root	421.53 MB	421.53 MB	1
rollup-tx-base-private	1070 MB	1070 MB	1
rollup-tx-base-public	3030 MB	3030 MB	1
rollup-tx-merge	336.24 MB	336.24 MB	1
semaphore_depth_10	98.11 MB	98.11 MB	1
sha512_100_bytes	185.96 MB	186.02 MB	1.00

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

Execution Memory

Details

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
private-kernel-inner	270.32 MB	270.32 MB	1
private-kernel-reset	306.03 MB	306.03 MB	1
private-kernel-tail	257.53 MB	257.53 MB	1
rollup-block-root	337.65 MB	337.65 MB	1
rollup-checkpoint-merge	336.48 MB	336.48 MB	1
rollup-checkpoint-root-single-block	1750 MB	1750 MB	1
rollup-checkpoint-root	1750 MB	1750 MB	1
rollup-root	360.88 MB	360.88 MB	1
rollup-tx-base-private	525.38 MB	525.38 MB	1
rollup-tx-base-public	466.5 MB	466.5 MB	1
rollup-tx-merge	335.72 MB	335.72 MB	1
semaphore_depth_10	74.19 MB	74.19 MB	1
sha512_100_bytes	72.33 MB	72.33 MB	1

This comment was automatically generated by workflow using github-action-benchmark.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: d11ee29	Previous: e658317	Ratio
sha512-100-bytes	0.023 s	0.017 s	1.35

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[aakoshh]

LGTM, apart from clarifying some comments.

Last week I was trying to come up with an example to convince myself that ArraySet needs inc_rc. I think it went something like this:

let mut a = [[1, 2], [3, 4]];
for i in 0..2 {
  for j in 0..2 {
    a[i] = [5, 6];
    a[i][j] = 7; 
    println(a);
  }
}

My thinking was that LICM would hoist a[i] out of the inner loop, thus it would only be set once, and then the inner loop would modify the [5, 6] array, overwriting one item and then the other, with the previous iteration's changes visible, and the prints eventually becoming all 7s. By leaving an inc_rc we make sure that the assignment to a[i][j] acts on a fresh copy of a[i] every time.

But this is not a good example, as any wrongdoing is currently prevented by the ownership analysis monomorphizing to this:

a$l0[i$l1].clone()[j$l2] = 7;

[guipublic]

I could not either get a failure with ArraySet. As you say, it is managed by the ownership analysis pass, so I have excluded it.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 7bc330a	Previous: 2c2a7b8	Ratio
semaphore-depth-10	0.033 s	0.027 s	1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 05bbbd6	Previous: 2c2a7b8	Ratio
semaphore-depth-10	0.033 s	0.027 s	1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench