feat: Optimize array sets in if conditions (alternate version)

[jfecher]

Description

Problem*

Alternate version of #4695

Resolves #4692

Summary*

• Flattening produces a new IfElse instruction to merge values instead of immediately merging them
• IfElse instructions are removed by actually merging the values once flattening and mem2reg is done. This is done in a new remove_if_else pass.
• Since slice capacity tracking was done in flattening before and required to merge values, I've had to move this to the remove_if_else pass instead. It is somewhat simpler since we don't have to track loads/stores.

Additional Context

Currently only failing the nested_array_in_slice and nested_array_dynamic tests.
The array being asserted somehow has the value [1, 2, 3] despite none of these values being set in the program.

There are currently no actual optimizations here, this is just structural changes so it should otherwise be equivalent.

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.

[jfecher]

Some improvements in circuit sizes so far despite me not having added the actual optimization yet.

[vezenovm]

The array being asserted somehow has the value [1, 2, 3] despite none of these values being set in the program.

Most likely the index flattening computation is just off by one somewhere. The first input to [Foo; 4] is the following:

[[x]]
a = "1"
b = ["2", "3", "20"]

so it may be fetching from where "a" starts rather than "b".

[jfecher]

Most likely the index flattening computation is just off by one somewhere

That seems likely, although odd since I haven't changed any of the merge values code.

Notably, adding a println of the value in certain spots also fixes the issue..

[vezenovm]

That seems likely, although odd since I haven't changed any of the merge values code.

I'm assuming this is at the beginning of those tests as well? Before any merges?

[jfecher]

At the beginning of the tests, println prints the correct value for

    assert(x[y - 1].bar.inner == [106, 107, 108]);

if x[y - 1].bar.inner is printed. If it is printed right before that assert it will print [1, 2, 3]. However, if another println is added between the two if statements in the program, all printouts are 106, 107, 108 and the program passes.

[jfecher]

Also, I'm looking at nested_array_in_slice, not nested_array_dynamic.

The problem persists if x is made into an array.

[vezenovm]

Also, I'm looking at nested_array_in_slice, not nested_array_dynamic.

Oh ok I was looking at nested_array_dynamic as I was more surprised that was failing as there are not any slices used at all.

[jfecher]

The smallest repro for the issue in the two failing tests I've been able to make so far is:

fn main(x: Field) {
    // x = 3
    let mut array = [(1, [2, 3], [4, 5, 6]), (7, [8, 9], [10, 11, 12])];

    if x != 1000 {
        array[x - 3].0 = 50;
    }

    if x == 1000 {
        array[x - 2].1 = [7, 8];
    } else {
        // x - 2 = 1
        println(array[x - 2].2);  // [50, 2, 3]
        println(array[1].2);      // [10, 11, 12]

        assert(array[1].2 == array[x - 2].2);
    }
}

Still unsure where exactly this error is coming from. I've double checked how indices are handled throughout the compiler including during value merging, the remove_if_else pass, and acir gen. I've also double checked the return types for array_get are correct so that it wouldn't cause another offset difference later.

[vezenovm]

The smallest repro for the issue in the two failing tests

I'll take a look as well. Thanks for the small repro

[jfecher]

Not sure if this matters (I think this is the issue) but one thing I've found comparing the SSA to the same code on master is that the array_get that returns the array[x - 2] array is done after an enable_side_effects_if on master, but before that same instruction in this PR. I believe that enable_side_effects_if is for the then case of if x == 1000 { which isn't chosen.

I think acir gen is multiplying the index here by that zero value. So that's why we'd return an array of the first three items instead.

[jfecher]

Alright, the reason why the ArrayGet is changed to an earlier location is that the constant folding pass is now replacing the second ArrayGet in a different EnableSideEffectsIf context with one used beforehand, in an EnableSideEffectsIf context that was disabled.

[jfecher]

Looks like the nested dynamic array test is still failing after the is_pure change.
Adding another array of indirection breaks this PR as well:

fn main(x: Field) {
    // x = 3
    let mut array = [[(1, [2, 3], [4, 5, 6])], [(7, [8, 9], [10, 11, 12])]];
    let zero = 0;

    if x != 1000 {
        array[x - 3][zero].0 = 50;
    }

    if x == 1000 {
        array[x - 2][zero].1 = [7, 8];
    } else {
        // x - 2 = 1
        println(array[x - 2][zero].2);  // [50, 2, 3]
        println(array[1][zero].2);      // [10, 11, 12]

        assert(array[1][zero].2 == array[x - 2][zero].2);
    }
}

Output: Index out of bounds, array has size 12, but index was 12

[vezenovm]

We are all passing on e3d5474. I haven't had the chance to experiment with weird changes when we update the merge values bound @jfecher. I was thinking for now I would just clean this PR (there are some old comments) and we can merge as we do have ~20% ACIR opcodes improvement for hashmap. Then in a follow-up we can look at how to improve that merge indices iter bound.

[jfecher]

@vezenovm fixes LGTM. I agree, we can experiment with the optimization changes later - this PR has been up long enough.

I can't approve since it was originally my PR

[TomAFrench]

@vezenovm fyi on the merge conflicts.