Skip to content

fix(ssa_gen): Generate code for index before the collection#9332

Merged
jfecher merged 10 commits intomasterfrom
af/9329-fix-ssa-gen-index
Aug 4, 2025
Merged

fix(ssa_gen): Generate code for index before the collection#9332
jfecher merged 10 commits intomasterfrom
af/9329-fix-ssa-gen-index

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Jul 26, 2025

Description

Problem*

Resolves #9329

Summary*

Swaps the order in codegen_index to generate the index first and then the collection, so that if the collection is mutable and the index modifies it, the loading of the reference happens after the index is calculated.

Additional Context

This is what the initial SSA looked like originally:

acir(inline) fn main f0 {
  b0(v0: [i8; 2]):
    v2 = allocate -> &mut [i8; 2]
    store v0 at v2
    v3 = load v2 -> [i8; 2]
    jmp b1(u32 0)
  b1(v1: u32):
    ...          
    jmpif v6 then: b2, else: b3
  b2():
    ...                          	
    v13 = load v2 -> [i8; 2]                      	
    v14 = array_set v13, index u32 0, value v12   	
    store v14 at v2                               	
    ...
    jmp b1(v15)
  b3():
    v7 = array_get v3, index u32 0 -> i8          	
    return v7
}

Notice how we allocate a reference v2 to store the input array v0, which is immediately loaded into v3. Then we go into the loop where we load v2, modify it as v14 and store the new value. However in the final block b3, we get the return value v7 from the array v3 which we loaded in b0.

The reason it worked in Brillig was that Brillig modified the array in place, so even though we used v3, it reflected the effects of array_set v13. In ACIR we modified a copy instead, and then never loaded it again from the reference.

After the change in the PR, the new SSA moves the load into b3:

After Initial SSA:
acir(inline) fn main f0 {
  b0(v0: [i8; 2]):
    v2 = allocate -> &mut [i8; 2]
    store v0 at v2
    jmp b1(u32 0)
  b1(v1: u32):
    v5 = lt v1, u32 2                             	
    jmpif v5 then: b2, else: b3
  b2():
    v8 = load v2 -> [i8; 2]                       	
    v10 = array_get v8, index u32 1 -> i8         	
    v12 = add v10, i8 1                           	
    v13 = load v2 -> [i8; 2]                      	
    v14 = array_set v13, index u32 0, value v12   	
    store v14 at v2                               	
    v15 = unchecked_add v1, u32 1                 	
    jmp b1(v15)
  b3():
    v6 = load v2 -> [i8; 2]                       	
    v7 = array_get v6, index u32 0 -> i8          	
    return v7
}

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.

@aakoshh aakoshh requested a review from a team July 26, 2025 10:43
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: 5a69cbf Previous: 2282832 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 2 s 1 s 2
test_report_zkpassport_noir-ecdsa_ 3 s 1 s 3

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

CC: @TomAFrench

@aakoshh aakoshh force-pushed the af/9329-fix-ssa-gen-index branch from 6735d82 to 5cfb736 Compare July 26, 2025 11:41
Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Nice find!

@aakoshh aakoshh added this pull request to the merge queue Jul 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jul 28, 2025
@TomAFrench TomAFrench enabled auto-merge August 4, 2025 15:38
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: 41067ab Previous: 9b1b10f Ratio
sha512-100-bytes 0.134 s 0.104 s 1.29

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

CC: @TomAFrench

@TomAFrench TomAFrench added this pull request to the merge queue Aug 4, 2025
@jfecher jfecher removed this pull request from the merge queue due to a manual request Aug 4, 2025
@jfecher jfecher added this pull request to the merge queue Aug 4, 2025
Merged via the queue into master with commit b61f5eb Aug 4, 2025
122 checks passed
@jfecher jfecher deleted the af/9329-fix-ssa-gen-index branch August 4, 2025 16:29
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 11, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: Release Noir(1.0.0-beta.10)
(noir-lang/noir#9311)
feat(ssa_fuzzer): arrays support
(noir-lang/noir#9427)
chore: add link to issue on TODOs
(noir-lang/noir#8307)
fix(ownership): Increment reference count for nested array get in LHS
assignment (noir-lang/noir#9347)
chore: add some mem2reg unit tests
(noir-lang/noir#9405)
chore(as_slice_length): Various unit tests
(noir-lang/noir#9419)
chore(simplify_cfg): Additional unit tests
(noir-lang/noir#9426)
chore: create directory when writing witness artefact
(noir-lang/noir#9383)
fix: throw error if foreign call returns the wrong number of fields
(noir-lang/noir#9286)
chore: update ex in docs (noir-lang/noir#9385)
chore: add some `assert_constant` tests
(noir-lang/noir#9413)
chore: error on non constant inputs for Pedersen generators
(noir-lang/noir#9389)
chore(inlining): Unit tests for global values and conditional inlining
(noir-lang/noir#9411)
chore(ssa_fuzzer): refactor fuzzing modes + add fuzzing mode without DIE
pass (noir-lang/noir#9401)
chore(inlining): Various unit tests
(noir-lang/noir#9388)
feat(ssa_fuzzer): pushing generated program and witness to redis queue
(noir-lang/noir#9375)
fix(ssa_gen): Generate code for index before the collection
(noir-lang/noir#9332)
chore: Regression test for calling a mutable closure inside a mutable
closure (noir-lang/noir#9384)
fix: some nargo expand fixes
(noir-lang/noir#9324)
fix: disable comptime printing when requesting json output
(noir-lang/noir#9381)
chore: address TODO comments
(noir-lang/noir#9379)
feat: type alias for numeric generics
(noir-lang/noir#7583)
chore: bump external pinned commits
(noir-lang/noir#9291)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
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.

ACIR != Brillig: Assign to array inside the index expression with loop

4 participants