Skip to content

fix(acir-gen): Use the side effect variable in slice_pop_back#10455

Merged
jfecher merged 4 commits intomasterfrom
af/10446-slice-pop-back-zero-length
Nov 10, 2025
Merged

fix(acir-gen): Use the side effect variable in slice_pop_back#10455
jfecher merged 4 commits intomasterfrom
af/10446-slice-pop-back-zero-length

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 10, 2025

Description

Problem

Resolves #10446

Summary

Fixes the ACIR generation of the slice_pop_back operation to use the current_side_effect_var if the length of the slice is not a constant, to calculate new_length as (slice_length - 1) * side_effects_enabled instead of just slice_length - 1.

Additional Context

The flattening pass makes the slice length a variable which is calculated based on which branch of the program was taken. In the example in the ticket, we had code like this:

if !a {
  let mut s = [10];
  if a {
    s = [20, 30];
    let (_, _) = s.pop();
  } else {
    s = [40, 50, 60, 70];
  }
} else {
  ...
}

When we flatten this for ACIR, we end up with a slice backed by an array of length 4, to accommodate the longest alternative, and a length that is a multiplication of the different length options:

  • length = (1-a)*a*2 + (1-a)*(1-a)*4 + a*0 = (1-a)*4
  • array = [(1-a)*a*20 + (1-a)*(1-a)*40, (1-a)*a*30 + (1-a)*(1-a)*50, 60, 70] = [(1-a)*40, (1-a)*50, 60, 70]

When a is 1, the length becomes 0, and the array becomes [0, 0, 60, 70], which is what we see in #10446 (comment)

The problem is that slice_pop_back wants to read the item at length-1, ie. the last item, which for a slice of length 0 is item -1.

With the current change, it should read item 0.

I think reading item 0 should not fail: if a slice is known to be empty, the instruction would have already been eliminated as unreachable.

User Documentation

Check one:

  • No user documentation needed.
  • Changes in docs/ included in this PR.
  • [For Experimental Features] Changes in docs/ 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.

@github-actions
Copy link
Contributor

Changes to circuit sizes

Generated at commit: d2f70d2f63b800f396f7cc6ed49ccd964d07c255, compared to commit: e4228986ee37f02c0f55df04ec0e95d161955feb

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
slice_dynamic_index +18 ❌ +2.24% +18 ❌ +0.35%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
slice_dynamic_index 822 (+18) +2.24% 5,177 (+18) +0.35%

@aakoshh aakoshh requested review from a team and guipublic November 10, 2025 17:53
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: 5bb3ce6 Previous: e422898 Ratio
test_report_zkpassport_noir-ecdsa_ 2 s 1 s 2

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

CC: @TomAFrench

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher added this pull request to the merge queue Nov 10, 2025
Merged via the queue into master with commit 1bbaffe Nov 10, 2025
144 checks passed
@jfecher jfecher deleted the af/10446-slice-pop-back-zero-length branch November 10, 2025 20:17
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 11, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(brillig_gen): Switch to iterative variable liveness
(noir-lang/noir#10460)
feat: remove unnecessary mutation of blackbox functions during
flattening (noir-lang/noir#10182)
chore: revert "fix: revert "feat(ACIR): reuse element_type_sizes blocks
with… (noir-lang/noir#10461)
chore: green light for acir (native_types) audit
(noir-lang/noir#10381)
chore: lock Cargo.lock in cargo-binstall
(noir-lang/noir#10459)
fix(acir-gen): Use the side effect variable in `slice_pop_back`
(noir-lang/noir#10455)
fix: correct location for out of bounds match case integer
(noir-lang/noir#10454)
fix: Defunctionalize foreign functions in pre-SSA pass over mAST
(noir-lang/noir#10160)
fix: use unit for fmtstr without variables
(noir-lang/noir#10456)
chore(docs): Update tinyjs app tutorial for versioned docs
(noir-lang/noir#10453)
fix(frontend): Resolve to correct type on fmtstr interpolation error
(noir-lang/noir#10450)
fix: avoid producing duplicate private error messages
(noir-lang/noir#10449)
chore(docs): update dependencies and installation instructions in NoirJS
tutorial and examples (noir-lang/noir#10400)
fix(compiler): Improve error message for impl on primitive types
(noir-lang/noir#10430)
(noir-lang/noir#10442)
chore: get trait as non-mut
(noir-lang/noir#10447)
chore(frontend): Tuple dereference chain unit test and minor method
reorg (noir-lang/noir#10410)
fix(doc): analyze sub-modules imports before self
(noir-lang/noir#10390)
chore: green light for blackbox_solver audit
(noir-lang/noir#10372)
chore: use `get_last_condition` in `link_condition`
(noir-lang/noir#10424)
chore: bump external pinned commits
(noir-lang/noir#10443)
feat: primitive types doc comments
(noir-lang/noir#10432)
chore(frontend): Trait impl Self path unit tests
(noir-lang/noir#10437)
END_COMMIT_OVERRIDE
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 11, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(brillig_gen): Switch to iterative variable liveness (noir-lang/noir#10460)
feat: remove unnecessary mutation of blackbox functions during flattening (noir-lang/noir#10182)
chore: revert "fix: revert "feat(ACIR): reuse element_type_sizes blocks with… (noir-lang/noir#10461)
chore: green light for acir (native_types) audit (noir-lang/noir#10381)
chore: lock Cargo.lock in cargo-binstall (noir-lang/noir#10459)
fix(acir-gen): Use the side effect variable in `slice_pop_back` (noir-lang/noir#10455)
fix: correct location for out of bounds match case integer (noir-lang/noir#10454)
fix: Defunctionalize foreign functions in pre-SSA pass over mAST (noir-lang/noir#10160)
fix: use unit for fmtstr without variables (noir-lang/noir#10456)
chore(docs): Update tinyjs app tutorial for versioned docs (noir-lang/noir#10453)
fix(frontend): Resolve to correct type on fmtstr interpolation error (noir-lang/noir#10450)
fix: avoid producing duplicate private error messages (noir-lang/noir#10449)
chore(docs): update dependencies and installation instructions in NoirJS tutorial and examples (noir-lang/noir#10400)
fix(compiler): Improve error message for impl on primitive types (noir-lang/noir#10430) (noir-lang/noir#10442)
chore: get trait as non-mut (noir-lang/noir#10447)
chore(frontend): Tuple dereference chain unit test and minor method reorg (noir-lang/noir#10410)
fix(doc): analyze sub-modules imports before self (noir-lang/noir#10390)
chore: green light for blackbox_solver audit (noir-lang/noir#10372)
chore: use `get_last_condition` in `link_condition` (noir-lang/noir#10424)
chore: bump external pinned commits (noir-lang/noir#10443)
feat: primitive types doc comments (noir-lang/noir#10432)
chore(frontend): Trait impl Self path unit tests (noir-lang/noir#10437)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 11, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(brillig_gen): Switch to iterative variable liveness
(noir-lang/noir#10460)
feat: remove unnecessary mutation of blackbox functions during
flattening (noir-lang/noir#10182)
chore: revert "fix: revert "feat(ACIR): reuse element_type_sizes blocks
with… (noir-lang/noir#10461)
chore: green light for acir (native_types) audit
(noir-lang/noir#10381)
chore: lock Cargo.lock in cargo-binstall
(noir-lang/noir#10459)
fix(acir-gen): Use the side effect variable in `slice_pop_back`
(noir-lang/noir#10455)
fix: correct location for out of bounds match case integer
(noir-lang/noir#10454)
fix: Defunctionalize foreign functions in pre-SSA pass over mAST
(noir-lang/noir#10160)
fix: use unit for fmtstr without variables
(noir-lang/noir#10456)
chore(docs): Update tinyjs app tutorial for versioned docs
(noir-lang/noir#10453)
fix(frontend): Resolve to correct type on fmtstr interpolation error
(noir-lang/noir#10450)
fix: avoid producing duplicate private error messages
(noir-lang/noir#10449)
chore(docs): update dependencies and installation instructions in NoirJS
tutorial and examples (noir-lang/noir#10400)
fix(compiler): Improve error message for impl on primitive types
(noir-lang/noir#10430)
(noir-lang/noir#10442)
chore: get trait as non-mut
(noir-lang/noir#10447)
chore(frontend): Tuple dereference chain unit test and minor method
reorg (noir-lang/noir#10410)
fix(doc): analyze sub-modules imports before self
(noir-lang/noir#10390)
chore: green light for blackbox_solver audit
(noir-lang/noir#10372)
chore: use `get_last_condition` in `link_condition`
(noir-lang/noir#10424)
chore: bump external pinned commits
(noir-lang/noir#10443)
feat: primitive types doc comments
(noir-lang/noir#10432)
chore(frontend): Trait impl Self path unit tests
(noir-lang/noir#10437)
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: Index out of bounds, array has size 8, but index was -1

2 participants