fix(acir-gen): Do not generate ACIR array opcodes when the length is zero

[aakoshh]

Description

Problem*

Resolves #9174

Summary*

Changes handle_array_operations in ACIR generation to skip generating opcodes for accessing an array when know that the array/slice has a zero size.

Additional Context

The code in question has a global zero sized slice, which it accesses in both branches of an if expression:

global G_A: [Field] = &[];
fn main(a: bool, b: bool, c: Field) -> pub (Field, Field) {
    if a {
        (
            {
                let i = G_A;
                i[(2050918985_u32 % i.len())]
            },
            ...,
        )
    } else {
        (
            {
                let k = G_A;
                k[(779011912_u32 % k.len())]
            }, c,
        )
    }
}

The final SSA began like this:

After Verifying no dynamic array indices to reference value elements (1) (step 40):
g0 = u32 0
g1 = make_array [] : [Field]

acir(inline) predicate_pure fn main f0 {
  b0(v2: u1, v3: u1, v4: Field):
    enable_side_effects v2                          // If `a` is true
    v6 = mod u32 2050918985, u32 0                	// 2050918985_u32 % i.len(), but only fail if `a` is true
    constrain u1 0 == v2, "Index out of bounds"   	// always fail if `a` is true, because the length is 0
    v8 = make_array [] : [Field]                  	// initialize an empty array for the slice
    v9 = array_get v8, index v6 -> Field          	// access the array, but by now we know we will have failed
    enable_side_effects u1 0
...

And the generated ACIR:

Compiled ACIR for main (unoptimized):
func 0                                              // main
...                            
private parameters indices : [_0, _1, _2]           // a, b, c
...
BLACKBOX::RANGE [(_1, 1)] []                        // `b` is bool
EXPR [ (1, _0) 0 ]                                  // because the modulo is known to fail if `a` is true, `a` _must_ be false for the circuit to succeed
INIT (id: 0, len: 0, witnesses: [])                 // initialize an empty array for the slice
EXPR [ (-1, _5) 0 ]                                 // create a witness for the index, reading from index 0
MEM (id: 0, read at: EXPR [ (1, _5) 0 ], value: EXPR [ (1, _6) 0 ]) // read an item from the empty array
EXPR [ -1 ]                                         // fail unconditionally
...

The program failed with the following error:

error: Index out of bounds, array has size 0, but index was 0
  ┌─ src/main.nr:7:17
  │
7 │                 i[(2050918985_u32 % i.len())]

So this branch should never have run, but the MEM opcode fails anyway, because even though it looks at the side effect variable to decide whether to get/set items, it always expected a non-empty array.

With the changes in the PR, this is now eliminated:

Compiled ACIR for main (unoptimized):
func 0
...
BLACKBOX::RANGE [(_1, 1)] []
EXPR [ (1, _0) 0 ]
EXPR [ -1 ]
...

In the original approach of the PR the elimination only happened on the top branch, where it was obvious that the code would never run, because v2 was constrained to be 0. Then I changed it to happen whenever we know the array/slice is empty, which includes the else branch:

g0 = u32 0
g1 = make_array [] : [Field]

acir(inline) predicate_pure fn main f0 {
  b0(v2: u1, v3: u1, v4: Field):
...
    v17 = not v2
...
    enable_side_effects v17
    v27 = mod u32 779011912, u32 0                	// src/main.nr:27:20
    constrain v2 == u1 1, "Index out of bounds"   	// src/main.nr:27:20
    v29 = array_get v8, index v27 -> Field        	// src/main.nr:27:17
    enable_side_effects u1 1
...
    return v34, v37
}

in ACIR this no longer has memory operations either:

...
BRILLIG CALL func 1: inputs: [Single(Expression { mul_terms: [], linear_combinations: [], q_c: 779011912 }), Single(Expression { mul_terms: [], linear_combinations: [], q_c: 0 })], outputs: [Simple(Witness(5)), Simple(Witness(6))]
BLACKBOX::RANGE [(_5, 33)] []
BLACKBOX::RANGE [(_6, 0)] []
EXPR [ (-1, _7) 0 ]
BLACKBOX::RANGE [(_7, 0)] []
EXPR [ (-1, _6) 779011912 ]
EXPR [ -1 ]
EXPR [ 1 ]
EXPR [ (1, _3) 0 ]
EXPR [ (-1, _2) (1, _4) 0 ]
...

So we call Brillig to calculate the modulo (we can look separately into why the compiler did not eliminate this call), then fail unconditionally.

While the reason for the presence of the Brillig call is still something we can work out, the fix should unblock the PRs that fail due to the difference in ACIR and Brillig.

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.

[TomAFrench]

Looks like this covers #9193, shall we swap out the regression test to have something simpler? ah looks like the case is slightly different as the indexing is in both branches.

[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: bc10d91	Previous: 9efc476	Ratio
test_report_zkpassport_noir_rsa_	2 s	1 s	2

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

CC: @TomAFrench

[TomAFrench]

While the reason for the presence of the Brillig call is still something we can work out, the fix should unblock the PRs that fail due to the difference in ACIR and Brillig.

Yeah, I agree that this isn't the best solution for this issue but it's relatively light touch so if it unblocks master then I'm happy to merge it as a stopgap.

I think the ideal solution on this is to handle this in SSA where I think we can be more aggressive on marking certain instructions as unreachable.

[aakoshh]

I checked locally that it does unblock #9108 and #9061

I agree it would be nice to make unreachable instruction removal take care of this. As discussed it has a bit of cascading effect at the moment: maybe we could replace variables with defaults that are defined until the next side effect variable, or do it earlier, but then it disrupted flattening. @asterite was looking at it.

[jfecher]

#9174

Apologies for missing this earlier, but it looks like this PR #9050 also tackles the problem of changing acri-gen's behavior to not do array[index * predicate] for 0-length arrays

Changes handle_array_operations in ACIR generation to skip generating opcodes for accessing an array when i) we know that the side effect variable is zero and ii) the array/slice has a zero size.

We should also change the behavior not to do array[index * predicate] even if the predicate is not known to be zero

[aakoshh]

@jfecher thanks for the pointer to Guillaume's PR, I changed it to work like his by inserting an assertion that the side effect variable is false, and doing it whenever the flattened_size is zero, which covers slices and arrays as well. Not sure if I'm not inviting differences with the interpreter this way, let's see.

[jfecher]

which covers slices and arrays as well. Not sure if I'm not inviting differences with the interpreter this way, let's see.

maybe, but we can leave it to the other PR to resolve those differences at least