Skip to content

fix(interpreter): Return fresh nested array on array get in ACIR#9831

Merged
vezenovm merged 2 commits intomv/do-not-clone-index-call-argsfrom
mv/array-get-fresh-nested-array-interpreter-acir
Sep 15, 2025
Merged

fix(interpreter): Return fresh nested array on array get in ACIR#9831
vezenovm merged 2 commits intomv/do-not-clone-index-call-argsfrom
mv/array-get-fresh-nested-array-interpreter-acir

Conversation

@vezenovm
Copy link
Contributor

Description

Problem*

Resolves #9830

This will unblock adding the tests in #9789.

Summary*

When fetching arrays in ACIR we treat them as entirely fresh objects. However, in the interpreter we implement arrays use shared mutable references. Thus, when we returned a nested array and then marked it mutable in the array set optimization we were in fact mutating the parent array we fetched from as well. This breaks the semantics of arrays in ACIR.

Looking at the SSA after the array set optimization:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u1, v1: u32):
    v4 = make_array [u1 1, u1 0, u1 1] : [u1; 3]
    v5 = make_array [v4] : [[u1; 3]; 1]
    v7 = add v1, u32 1
    v8 = array_get v5, index v1 -> [u1; 3]
    v9 = array_set mut v8, index u32 1, value u1 1
    v10 = array_get v9, index v7 -> u1
    enable_side_effects v0
    v11 = add v1, u32 1
    v12 = array_get v5, index v1 -> [u1; 3]
    v13 = array_get v12, index v11 -> u1
    v14 = not v0
    enable_side_effects u1 1
    v15 = unchecked_mul v0, v13
    v16 = unchecked_mul v14, v10
    v17 = unchecked_add v15, v16
    return v17
}

We expect v8 to be its own fresh array. I changed the interpreter to check if we are returned an array from an array get and if we are to clone its elements and construct a new shared mutable reference to its elements.

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.

@vezenovm vezenovm requested a review from a team September 11, 2025 18:30
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: 67a534c Previous: b74c482 Ratio
test_report_noir-lang_noir_bigcurve_ 424 s 328 s 1.29
test_report_zkpassport_noir-ecdsa_ 3 s 1 s 3

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

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: d2525c2 Previous: 2a5315c Ratio
rollup-root 0.005 s 0.004 s 1.25

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

CC: @TomAFrench

@vezenovm vezenovm requested review from a team and jfecher September 11, 2025 22:06
@vezenovm vezenovm merged commit ff9b9e1 into mv/do-not-clone-index-call-args Sep 15, 2025
119 checks passed
@vezenovm vezenovm deleted the mv/array-get-fresh-nested-array-interpreter-acir branch September 15, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants