Skip to content

fix(acir_gen): A slice might be a nested Array, not a flattened DynamicArray#9600

Merged
aakoshh merged 3 commits intomasterfrom
af/9593-fix-acir-pop-front
Aug 26, 2025
Merged

fix(acir_gen): A slice might be a nested Array, not a flattened DynamicArray#9600
aakoshh merged 3 commits intomasterfrom
af/9593-fix-acir-pop-front

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Aug 21, 2025

Description

Problem*

Resolves #9593

Summary*

Fixes convert_ssa_intrinsic_call handling SlicePopFront where it calculated the number of items it needs to pop wrong: it assumed the slice will be a flattened DynamicArray, but in this case it was an Array with a nested structure. Based on this assumption it popped as many elements as were in the item, rather than just 1.

Additional Context

This is what the slice looked like as an AcirValue:

            Array([
                Array([
                    Var(AcirVar(30), NumericType(Unsigned { bit_size: 8 })), 
                    Var(AcirVar(38), NumericType(Unsigned { bit_size: 8 }))
                ]), 
                Array([
                    Var(AcirVar(47), NumericType(Unsigned { bit_size: 8 })), 
                    Var(AcirVar(55), NumericType(Unsigned { bit_size: 8 }))
                ]), 
                Array([
                    Var(AcirVar(59), NumericType(Unsigned { bit_size: 8 })), 
                    Var(AcirVar(62), NumericType(Unsigned { bit_size: 8 }))
                ])
            ])

The routine calculated that each item consists of 2 characters, and therefore it popped the first 2 arrays, leaving new_slice to be just a single array:

[
    Array([
        Var(AcirVar(59), NumericType(Unsigned { bit_size: 8 })), 
        Var(AcirVar(62), NumericType(Unsigned { bit_size: 8 }))
    ])]

I have not tried if any of the other operations have a similar assumption. I think I'll add Insert and Remove to the fuzzer to cover them, as they have quite substantial ACIR gen logic.

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 August 21, 2025 15:34
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: aed4ebd Previous: ef436d0 Ratio
private-kernel-inner 0.018 s 0.014 s 1.29

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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: aed4ebd Previous: ef436d0 Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 2 s 1.50
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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

CC: @TomAFrench

@aakoshh aakoshh requested a review from vezenovm August 26, 2025 08:43
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

Looks good! Would be good to get similar tests for insert and remove as well as you mentioned in the OP.

@aakoshh aakoshh added this pull request to the merge queue Aug 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 26, 2025
@aakoshh aakoshh added this pull request to the merge queue Aug 26, 2025
Merged via the queue into master with commit 29b3639 Aug 26, 2025
124 checks passed
@aakoshh aakoshh deleted the af/9593-fix-acir-pop-front branch August 26, 2025 16:29
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily (noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests (noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained, unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence of nested references  (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions (noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument (noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613 (noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit) (noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown alias set with nested references (noir-lang/noir#9632)
chore: add a regression test for #9613 (noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen (noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests (noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions (noir-lang/noir#9624)
chore: some inlining refactors (noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation (noir-lang/noir#9620)
chore: bump external pinned commits (noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure (noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits (noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases (noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg (noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
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: pop_front removes 2 items and leaves in default in ACIR

2 participants