fix(acir_gen): Handle flattening of numeric types when an Array contains a DynamicArray

[aakoshh]

Description

Problem*

Resolves #9860

Summary*

Changes handle_constant_index in acir::arrays so that if we were attempting to store a DynamicArray inside an Array, then we back out and take the regular non-constant route instead, which properly handles dynamic array values.

Superseded by #9894

Additional Context

The problem is similar to #9259

In the regression test added by the PR, we have the following SSA:

acir(inline) predicate_pure fn main f0 {
  b0(v0: [[u32; 4]; 4]):
    v2 = array_get v0, index u32 0 -> [u32; 4]
    v3 = array_get v2, index u32 0 -> u32
    v5 = array_get v0, index u32 2 -> [u32; 4]
    v7 = array_get v5, index u32 3 -> u32
    v8 = array_get v5, index u32 2 -> u32
    v9 = cast v7 as Field
    range_check v9 to 2 bits, "Index out of bounds"
    v10 = array_set mut v5, index v7, value v8
    v11 = array_set mut v0, index u32 2, value v10
    v12 = array_get v11, index v3 -> [u32; 4]
    v14 = array_set mut v12, index u32 1, value u32 0
    v15 = cast v3 as Field
    range_check v15 to 2 bits, "Index out of bounds"
    v16 = array_set mut v11, index v3, value v14
    return v16
}

Things go wrong when we are generating code for

    v10 = array_set mut v5, index v7, value v8
    v11 = array_set mut v0, index u32 2, value v10

In Noir this was a[2][idx_f] = a[2][2];, so first we set a value in the nested array, which returns v10, which we write back into the other array using a constant index 2.

The effect is that the result of the 2nd array_set is defined like this:

Array([
  Array([Var(AcirVar(1), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(3), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(5), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(7), NumericType(Unsigned { bit_size: 32 }))]), 
  Array([Var(AcirVar(9), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(11), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(13), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(15), NumericType(Unsigned { bit_size: 32 }))]), 
  DynamicArray(id: 1, len: 4, element_type_sizes: None), 
  Array([Var(AcirVar(25), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(27), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(29), NumericType(Unsigned { bit_size: 32 })), Var(AcirVar(31), NumericType(Unsigned { bit_size: 32 }))])
])

The exception came later when we then try to handle array_set mut v11, index v3, value v14. This time it's not a constant index, so it goes via array_set, and fails to flatten, because a DynamicArray in an Array is unexpected.

We would not have had a DynamicArray inside the Array if we allowed array_set_value to handle this by resolving the items, but handle_constant_index bypassed that.

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]

I'm not going to be at my computer for the rest of this evening (so can't validate) but I feel like we should be able to support this.

We can convert to dynamic array to a normal array by just laying down the memops necessary to read all the values so having a dynamic array within an array should not prevent flattening that array.

[TomAFrench]

I'm not 100% sure however that this would ever happen in a situation that wouldn't be optimized out by SSA so it may be a moot point.

[aakoshh]

Well something is clearly not expecting this, so I'm just applying the same backoff from a bad situation as before. I'm sure it can be handled, since last time the situation was that we had Array([Array(...), DynamicArray(...), Array(...)], but when we went into the handle_constant_index, it did not initialise the dynamic part, just returned it as-is, whereas array_get get handled it. So it can be done, I just didn't want to get bogged down trying to figure out how.

I'm not sure if the situation in the previous instance wasn't a result of the array_set being shortcut by handle_constant_index, similar to here, but there what failed was a follow up get, whereas now we have a follow up set. Maybe this fix could have precluded that fix.

Perhaps this is something to look into while greenlightling the ACIR gen, and we can do this simple fix now?

[vezenovm]

As Tom said we should be able to convert a dynamic array to a regular array by laying down the appropriate opcodes so escaping here feels strange.

(Reducing the ACIR gen complexity is just another reason to do #6231 as well)

[vezenovm]

Approving now that #9894 was merged into this branch

[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: cce4c10	Previous: a2ec2e5	Ratio
test_report_zkpassport_noir-ecdsa_	2 s	1 s	2

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

CC: @TomAFrench