fix(ownership): Increment reference count for nested array get in LHS assignment

[vezenovm]

Description

Problem*

Resolves #9270

Alternative to #9341

This is the AST we send to SSA gen:

global G_B$g0: [[str<2>; 1]; 2] = [["DS"], ["EN"]];
unconstrained fn main$f0() -> pub [[str<2>; 1]; 2] {
    {
        func_2$f1(G_B$g0.clone())
    };;
    G_B$g0.clone()
}
unconstrained fn func_2$f1(mut b$l0: [[str<2>; 1]; 2]) -> () {
    let mut idx_d$l1 = 0;
    loop {
        if (idx_d$l1 == 1) {
            break
        } else {
            idx_d$l1 = (idx_d$l1 + 1);
            b$l0[0][0] = G_B$g0[1].clone()[0].clone()
        }
    }
}

This produces the following SSA:

Details

brillig(inline) fn main f0 {
  b0():
    inc_rc g8
    call f1(g8)
    inc_rc g8
    return g8
}
brillig(inline) fn func_2 f1 {
  b0(v9: [[[u8; 2]; 1]; 2]):
    v10 = allocate -> &mut [[[u8; 2]; 1]; 2]
    store v9 at v10
    v11 = allocate -> &mut u32
    store u32 0 at v11
    jmp b1()
  b1():
    v13 = load v11 -> u32
    v15 = eq v13, u32 1
    jmpif v15 then: b3, else: b4
  b2():
    return
  b3():
    jmp b2()
  b4():
    v16 = load v11 -> u32
    v17 = add v16, u32 1
    store v17 at v11
    inc_rc g7
    inc_rc g6
    v18 = load v10 -> [[[u8; 2]; 1]; 2]
    v19 = array_get v18, index u32 0 -> [[u8; 2]; 1]
    v20 = array_set v19, index u32 0, value g6
    v21 = array_set v18, index u32 0, value v20
    store v21 at v10
    jmp b5()
  b5():
    jmp b1()
}

We appropriately clone g8 but that does not mean we updated the RCs of the internal arrays. So when we do v19 = array_get v18, index u32 0 -> [[u8; 2]; 1] the internal array has a ref count of one and we mutate it. So then b[0][0] gets changed to "EN" but so does G_B.

Summary*

It felt weird to "clone" the lhs assignment, as we don't necessarily want to clone b. We want to increment its RC because it is shared with another array pointer which is the array we actually want to clone. Thus, I chose to increment the RC during SSA gen when extracting the current value of the LHS for an assignment.

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 number of Brillig opcodes executed

Generated at commit: 83a540574bbcc2502e6796db6f006947d5cea2cb, compared to commit: c121513e7b2a3971cacf28f41eebf546918ef7e5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_9102_inliner_max	+33 ❌	+26.61%
regression_9102_inliner_zero	+33 ❌	+26.61%
regression_9102_inliner_min	+33 ❌	+24.26%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_9102_inliner_max	157 (+33)	+26.61%
regression_9102_inliner_zero	157 (+33)	+26.61%
regression_9102_inliner_min	169 (+33)	+24.26%
regression_struct_array_conditional_inliner_max	1,148 (+18)	+1.59%
regression_struct_array_conditional_inliner_min	1,148 (+18)	+1.59%
regression_struct_array_conditional_inliner_zero	1,148 (+18)	+1.59%
regression_9160_inliner_max	414 (+3)	+0.73%
regression_9160_inliner_min	414 (+3)	+0.73%
regression_9160_inliner_zero	414 (+3)	+0.73%
nested_array_dynamic_inliner_max	2,705 (+12)	+0.45%
nested_array_dynamic_inliner_min	2,889 (+12)	+0.42%
nested_array_dynamic_inliner_zero	2,889 (+12)	+0.42%

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 83a540574bbcc2502e6796db6f006947d5cea2cb, compared to commit: c121513e7b2a3971cacf28f41eebf546918ef7e5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_9102_inliner_max	+3 ❌	+2.33%
regression_9102_inliner_zero	+3 ❌	+2.33%
regression_9102_inliner_min	+3 ❌	+2.19%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_9102_inliner_max	132 (+3)	+2.33%
regression_9102_inliner_zero	132 (+3)	+2.33%
regression_9102_inliner_min	140 (+3)	+2.19%
regression_struct_array_conditional_inliner_max	390 (+6)	+1.56%
regression_struct_array_conditional_inliner_min	390 (+6)	+1.56%
regression_struct_array_conditional_inliner_zero	390 (+6)	+1.56%
nested_array_dynamic_inliner_min	1,449 (+21)	+1.47%
nested_array_dynamic_inliner_zero	1,449 (+21)	+1.47%
nested_array_dynamic_inliner_max	1,789 (+21)	+1.19%
regression_9160_inliner_max	367 (+3)	+0.82%
regression_9160_inliner_min	367 (+3)	+0.82%
regression_9160_inliner_zero	367 (+3)	+0.82%

[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: 5785ece	Previous: 08a955c	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	1 s	3

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

CC: @TomAFrench

[jfecher]

One of the goals of the ownership pass was to have all clones be explicit so they are easier to track compared to before when it was unclear when something was getting cloned.

Looking at the lvalue of b$l0[0][0] = G_B$g0[1].clone()[0].clone() and the fact we always inc_rc now in a nested array get, can you instead add a Clone node in the LValue enum that we always issue for nested array gets during the ownership pass instead?

[jfecher]

Resolves #9227
Resolves #9224
Resolves #9370

[asterite]

Resolves #9368

[jfecher]

Thank you for the changes - just how I imagined them

Some small regressions here but it is to be expected as we're inc_rcing more which is necessary for correctness.

[asterite]

I'm about to push some more new snapshots updates, in case anyone is also doing the same. There are a couple of failures but I think they are in tests that explicitly check what the reference count is at runtime, and these might have changed because of this PR... so they should be easy to fix.

[asterite]

Not sure what's going on with "Brillig execution trace sizes"...

[TomAFrench]

If we're going to merge this, can you update the timeouts to avoid CI failing?

[jfecher]

Blocking this while I investigate why the lvalue change caused brillig opcodes executed to increase so dramatically