fix(ownership): Clone global arrays

[vezenovm]

Description

Problem*

Resolves #8259

Taking the snippet from the issue:

global G_C: [bool; 3] = [true, false, true];
fn main(a: bool) -> pub [bool; 3] {
    let b = func_1(a, G_C);
    if a {
        G_C
    } else {
        b
    }
}
fn func_1(a: bool, mut b: [bool; 3]) -> [bool; 3] {
    b[1] = a;
    b
}

We have the following SSA:

g0 = u1 1
g1 = u1 0
g2 = make_array [u1 1, u1 0, u1 1] : [u1; 3]

brillig(inline) predicate_pure fn main f0 {
  b0(v3: u1):
    v5 = call f1(v3, g2) -> [u1; 3]
    v6 = not v3
    v7 = if v3 then g2 else (if v6) v5
    return v7
}
brillig(inline) predicate_pure fn func_1 f1 {
  b0(v3: u1, v4: [u1; 3]):
    v6 = array_set v4, index u32 1, value v3
    return v6
}

The crux of the issue is a missing inc_rc on g2. This indicates a bug in the ownership pass and that we should clone the global.

Summary*

We now clone any globals which contain arrays.

The snippet above produces the following SSA:

g0 = u1 1
g1 = u1 0
g2 = make_array [u1 1, u1 0, u1 1] : [u1; 3]

brillig(inline) predicate_pure fn main f0 {
  b0(v3: u1):
    inc_rc g2
    v5 = call f1(v3, g2) -> [u1; 3]
    inc_rc g2
    v6 = not v3
    v7 = if v3 then g2 else (if v6) v5
    return v7
}
brillig(inline) predicate_pure fn func_1 f1 {
  b0(v3: u1, v4: [u1; 3]):
    v6 = array_set v4, index u32 1, value v3
    return v6
}

Additional Context

This PR initially considering optimizing this process by initializing global arrays to have an RC > 1. This was chosen as unideal as it complicates array initialization in Brillig gen. It also essentially means our monomorphization output is false and being correct by Brillig gen. You can follow the discussion on this PR for why we moved away from this optimization to simplfy always cloning global arrays.

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: 7d54ae8b49f8484830ebeab788c8d4d1f09a84c1, compared to commit: b2d3a42c638536ad982ebdea0dbe2ac63e5f6e8b

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_8174_inliner_max	+33 ❌	+12.79%
regression_8174_inliner_min	+33 ❌	+12.79%
regression_8174_inliner_zero	+33 ❌	+12.79%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_8174_inliner_max	291 (+33)	+12.79%
regression_8174_inliner_min	291 (+33)	+12.79%
regression_8174_inliner_zero	291 (+33)	+12.79%
strings_inliner_max	1,524 (+3)	+0.20%
strings_inliner_zero	1,816 (+3)	+0.17%
strings_inliner_min	2,463 (+3)	+0.12%

[vezenovm]

I think it may be best to simply update the ownership pass. It would simplify Brillig gen and how we initialize arrays.

[jfecher]

I think it may be best to simply update the ownership pass. It would simplify Brillig gen and how we initialize arrays.

I think so too. It is an easy change in should_clone_ident to add the global case:

    fn should_clone_ident(&self, ident: &Ident) -> bool {
        match &ident.definition {
            Definition::Local(local_id) => {
                contains_array_or_str_type(&ident.typ) && !self.should_move(*local_id, ident.id)
            },
            // Globals are always cloned if they contain arrays
            Definition::Global(_) => contains_array_or_str_type(&ident.typ),
            _ => false,
        }
    }

This PRs version with starting RC of globals at 2 could be more efficient but I don't like that it means our monomorphized output is essentially incorrect and is relying on brillig to later correct it. For debugging in the future it can also complicate the mental model if we have to think about global arrays in SSA differently from normal arrays.

[jfecher]

Aside: all the snap changes make this PR look much larger than it actually is

[vezenovm]

I think so too. It is an easy change in should_clone_ident to add the global case:

Yeah that is what I did initially. I would like to track globals in last uses as well, although that should not be too bad to implement as it will copy what exists for local variables. For example in the code posted in the original issue we would get this SSA:

g0 = u1 1
g1 = u1 0
g2 = make_array [u1 1, u1 0, u1 1] : [u1; 3]

brillig(inline) predicate_pure fn main f0 {
  b0(v3: u1):
    inc_rc g2
    v5 = call f1(v3, g2) -> [u1; 3]
    inc_rc g2
    v6 = not v3
    v7 = if v3 then g2 else (if v6) v5
    return v7
}
brillig(inline) predicate_pure fn func_1 f1 {
  b0(v3: u1, v4: [u1; 3]):
    v6 = array_set v4, index u32 1, value v3
    return v6
}

The second inc_rc g2 is unneeded.

Aside: all the snap changes make this PR look much larger than it actually is

Yes, it is a bit unfortunate.

[vezenovm]

I would like to track globals in last uses as well

For now, I will make this a follow-up as to fix the issue first.

[jfecher]

I would like to track globals in last uses as well

We shouldn't track globals in last_uses since we should never move globals values. We don't know the ordering globals are used in in-between function calls which prevents us from determining where they're last used generally. If we limit the analysis to only entry point functions, these can be called recursively. I think it'd be a lot of effort to remove just 1 inc_rc from each global in the best case.

[vezenovm]

We shouldn't track globals in last_uses since we should never move globals values. We don't know the ordering globals are used in in-between function calls which prevents us from determining where they're last used generally. If we limit the analysis to only entry point functions, these can be called recursively. I think it'd be a lot of effort to remove just 1 inc_rc from each global in the best case.

Ah yes! 🤦 Good point that would be way too much effort for the benefits. Makes things easier!

[vezenovm]

@jfecher this is ready for review again. As you said the actual changes are very small and almost all of the diff are snapshots.

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 7d54ae8b49f8484830ebeab788c8d4d1f09a84c1, compared to commit: b2d3a42c638536ad982ebdea0dbe2ac63e5f6e8b

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_8174_inliner_max	+3 ❌	+1.68%
regression_8174_inliner_min	+3 ❌	+1.68%
regression_8174_inliner_zero	+3 ❌	+1.68%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_8174_inliner_max	182 (+3)	+1.68%
regression_8174_inliner_min	182 (+3)	+1.68%
regression_8174_inliner_zero	182 (+3)	+1.68%
strings_inliner_zero	912 (+3)	+0.33%
strings_inliner_max	917 (+3)	+0.33%
strings_inliner_min	1,045 (+3)	+0.29%

[jfecher]

LGTM