Skip to content

fix(ssa): Replace pop from 0-length slice with constraint and defaults#9489

Merged
aakoshh merged 10 commits intomasterfrom
af/9467-side-effect-pop-2
Aug 14, 2025
Merged

fix(ssa): Replace pop from 0-length slice with constraint and defaults#9489
aakoshh merged 10 commits intomasterfrom
af/9467-side-effect-pop-2

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Aug 14, 2025

Description

Problem*

Resolves #9467
Replaces #9471

Summary*

If we see a v1, v2, v3 = call slice_pop_front(u32 0, v0) which is statically known to fail, we replace it with:

  1. constrain 0 == <side-effect-variable>, "Index out of bounds" which will fails iff the pop was enabled, and
  2. v3 = make_array [] which creates a default value for the result, so that we can still compile and execute the SSA, without executing the pop, which would fail, because it ignores the side effect variable.

Additional Context

In #9471 my approach was to add the side effect variable as a predicate to the ACIR memory operation, turning it into a no-op if the branch was not enabled. However, Barretenberg doesn't handle memory operation predicates, so this was a no go.

In this PR we instead look for a constant zero length, and replace it with a constraint that fails when pop would have failed, but does not fail if the side effects are disabled. In both cases we can remove the pop.

We can only handle the case when we statically know that the slice is empty. If the length is a variable, and it depends on a condition, then it will still fail at runtime if it's empty, even if the side effects are disabled. In my testing I was able to create a situation where the length was a variable by merging slices of unequal length, but in these cases the storage was not empty, and it successfully popped even if the length was 0. For example let s: [u32] = if b { &[] } else { &[10] }) results in v0 = b * 1; v1 = make_array [b * 10], and despite length being 0, popping from [0] caused no "Index out of bounds".

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 marked this pull request as ready for review August 14, 2025 12:56
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: 8d4373e Previous: ee2ac1a Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 2 s 1.50
test_report_zkpassport_noir_rsa_ 1 s 0 s +∞

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.

We can only handle the case when we statically know that the slice is empty. If the length is a variable, and it depends on a condition, then it will still fail at runtime if it's empty, even if the side effects are disabled.

Can you make sure we have an issue for the runtime case?

@aakoshh
Copy link
Contributor Author

aakoshh commented Aug 14, 2025

67adec6 was required because of a fuzzing failure:

NOIR_AST_FUZZER_SEED=0x5624a20000100000 cargo test -q -p noir_ast_fuzzer_fuzz min_vs_full

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: 8d4373e Previous: ee2ac1a Ratio
private-kernel-inner 0.019 s 0.014 s 1.36

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

CC: @TomAFrench

@aakoshh aakoshh enabled auto-merge August 14, 2025 14:07
@aakoshh aakoshh added this pull request to the merge queue Aug 14, 2025
Merged via the queue into master with commit 20a6467 Aug 14, 2025
121 of 122 checks passed
@aakoshh aakoshh deleted the af/9467-side-effect-pop-2 branch August 14, 2025 14:42
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 15, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: restore noir libs in CI (noir-lang/noir#9499)
feat!: new semantic for bit-shifts  (noir-lang/noir#9373)
fix(ssa): Replace side effects with defaults when disabled (noir-lang/noir#9462)
fix(ssa): Replace pop from 0-length slice with constraint and defaults (noir-lang/noir#9489)
fix: assert types are not mutated in constant folding (noir-lang/noir#9481)
fix: remove shadowing in `BoundedVec::any` causing returning  false unconditionally (noir-lang/noir#9478)
END_COMMIT_OVERRIDE
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 15, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: restore noir libs in CI (noir-lang/noir#9499)
feat!: new semantic for bit-shifts  (noir-lang/noir#9373)
fix(ssa): Replace side effects with defaults when disabled (noir-lang/noir#9462)
fix(ssa): Replace pop from 0-length slice with constraint and defaults (noir-lang/noir#9489)
fix: assert types are not mutated in constant folding (noir-lang/noir#9481)
fix: remove shadowing in `BoundedVec::any` causing returning  false unconditionally (noir-lang/noir#9478)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 15, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: restore noir libs in CI
(noir-lang/noir#9499)
feat!: new semantic for bit-shifts
(noir-lang/noir#9373)
fix(ssa): Replace side effects with defaults when disabled
(noir-lang/noir#9462)
fix(ssa): Replace pop from 0-length slice with constraint and defaults
(noir-lang/noir#9489)
fix: assert types are not mutated in constant folding
(noir-lang/noir#9481)
fix: remove shadowing in `BoundedVec::any` causing returning false
unconditionally (noir-lang/noir#9478)
END_COMMIT_OVERRIDE
mralj pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 13, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: restore noir libs in CI (noir-lang/noir#9499)
feat!: new semantic for bit-shifts  (noir-lang/noir#9373)
fix(ssa): Replace side effects with defaults when disabled (noir-lang/noir#9462)
fix(ssa): Replace pop from 0-length slice with constraint and defaults (noir-lang/noir#9489)
fix: assert types are not mutated in constant folding (noir-lang/noir#9481)
fix: remove shadowing in `BoundedVec::any` causing returning  false unconditionally (noir-lang/noir#9478)
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.

Incorrect ACIR: pop_front fails when should not execute

2 participants