Skip to content

chore(acir): Test whether the predicate has an effect on slice intrinsics#8421

Merged
TomAFrench merged 4 commits intomasterfrom
mv/slice-intrinsic-acir-pred-tests
May 8, 2025
Merged

chore(acir): Test whether the predicate has an effect on slice intrinsics#8421
TomAFrench merged 4 commits intomasterfrom
mv/slice-intrinsic-acir-pred-tests

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented May 8, 2025

Description

Problem*

Resolves #8355 (comment)

Summary*

So although this resolves the comment on #8355, this functionality can be tested in isolation without considering that PR at all. The following was done to determine whether a predicate affected a certain instruction:

  1. Setup an SSA where there is only instruction after EnableSideEffectIf instruction on a dynamic predicate
  2. Check the ACIR and final witness index for an SSA where that EnableSideEffectIf is in place and an SSA where there is no EnableSideEffectIf.
  3. If the instruction is meant to be unaffected by the predicate, check for equality. If the predicate is meant to have an affect, check for inequality.

For example we may have this SSA:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u32, v1: u1):
    v4 = make_array [Field 2, Field 3] : [Field; 2]
    v6 = make_array [Field 1, v4] : [(Field, [Field; 2])]
    v8 = array_set v6, index v0, value Field 4
    enable_side_effects v1
    v11, v12 = call slice_push_back(u32 1, v8, Field 1, v4) -> (u32, [(Field, [Field; 2])])
    return
}

and we want to check that the ACIR generated is equivalent to this SSA:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u32, v1: u1):
    v4 = make_array [Field 2, Field 3] : [Field; 2]
    v6 = make_array [Field 1, v4] : [(Field, [Field; 2])]
    v8 = array_set v6, index v0, value Field 4
    v11, v12 = call slice_push_back(u32 1, v8, Field 1, v4) -> (u32, [(Field, [Field; 2])])
    return
}

Composite types were used for setting up the slice tests. This was necessary to guarantee going that slice_insert and slice_remove were affected by the predicate.

Additional Context

In general, I think this strategy of testing could be an effective simple way to test requires_acir_gen_predicate is in fact marked appropriately. We could expand it to the remaining intrinsic calls and instructions. If after a review we decide to go forward with these kinds of tests, I am happy to capture an issue for adding more ACIR gen predicate tests for the remaining instructions.

The helper in this PR is specific to setting up slice intrinsic calls, however, it could definitely be expand to support all intrinsic calls, and even all instructions (perhaps we will want a new helper at that point).

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.

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: d6847a3 Previous: 8ab59da Ratio
test_report_noir-lang_noir_bigcurve_ 309 s 230 s 1.34

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: a3b8d24 Previous: 9dc9cf6 Ratio
rollup-merge 0.004 s 0.003 s 1.33

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

CC: @TomAFrench

@TomAFrench TomAFrench added this pull request to the merge queue May 8, 2025
Merged via the queue into master with commit b42264b May 8, 2025
116 checks passed
@TomAFrench TomAFrench deleted the mv/slice-intrinsic-acir-pred-tests branch May 8, 2025 22:51
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: always type-check turbofish, and error when it's not allowed
(noir-lang/noir#8437)
chore: Release Noir(1.0.0-beta.5)
(noir-lang/noir#7955)
feat(greybox_fuzzer): Parallel fuzz tests
(noir-lang/noir#8432)
fix(ssa): Mislabeled instructions with side effects in
EnableSideEffectsIf removal pass
(noir-lang/noir#8355)
feat: SSA pass impact report
(noir-lang/noir#8393)
chore: bump external pinned commits
(noir-lang/noir#8433)
chore: separate benchmarking from github actions more
(noir-lang/noir#7943)
chore(fuzz): Break up the AST fuzzer `compare` module
(noir-lang/noir#8431)
chore(fuzz): Rename `init_vs_final` to `min_vs_full`
(noir-lang/noir#8430)
fix!: error on tuple mismatch
(noir-lang/noir#8424)
chore: bump external pinned commits
(noir-lang/noir#8429)
chore(acir): Test whether the predicate has an effect on slice
intrinsics (noir-lang/noir#8421)
feat(ssa): Mark transitively dead parameters during DIE
(noir-lang/noir#8254)
fix(ssa_gen): Do not code gen fetching of empty arrays when initializing
the data bus (noir-lang/noir#8426)
chore: remove `.aztec-sync-commit`
(noir-lang/noir#8415)
chore(test): Add more unit tests for
`inline_functions_with_at_most_one_instruction`
(noir-lang/noir#8418)
chore: add minor docs for interpreter
(noir-lang/noir#8397)
fix: print slice composite types surrounded by parentheses
(noir-lang/noir#8412)
feat: Skip SSA passes that contain any of the given messages
(noir-lang/noir#8416)
fix: disable range constraints using the predicate
(noir-lang/noir#8396)
chore: bumping external libraries
(noir-lang/noir#8406)
chore: redo typo PR by shystrui1199
(noir-lang/noir#8405)
feat(test): add `nargo_fuzz_target`
(noir-lang/noir#8308)
fix: allow names to collide in the values/types namespaces
(noir-lang/noir#8286)
fix: Fix sequencing of side-effects in lvalue
(noir-lang/noir#8384)
feat(greybox_fuzzer): Maximum executions parameter added
(noir-lang/noir#8390)
fix: warn on and discard unreachable statements after break and continue
(noir-lang/noir#8382)
fix: add handling for u128 infix ops in interpreter
(noir-lang/noir#8392)
chore: move acirgen tests into separate file
(noir-lang/noir#8376)
feat(fuzz): initial version of comptime vs brillig target for AST fuzzer
(noir-lang/noir#8335)
chore: apply lints to `ast_fuzzer`
(noir-lang/noir#8386)
chore: add note on AI generated PRs in `CONTRIBUTING.md`
(noir-lang/noir#8385)
chore: document flattening pass
(noir-lang/noir#8312)
fix: comptime shift-right overflow is zero
(noir-lang/noir#8380)
feat: let static_assert accept any type for its message
(noir-lang/noir#8322)
fix(expand): output safety comment before statements
(noir-lang/noir#8378)
chore: avoid need to rebuild after running tests
(noir-lang/noir#8379)
chore: bump dependencies (noir-lang/noir#8372)
chore: Add GITHUB_TOKEN to cross build
(noir-lang/noir#8370)
chore: redo typo PR by GarmashAlex
(noir-lang/noir#8364)
chore: remove unsafe code from greybox fuzzer
(noir-lang/noir#8315)
feat: add `--fuzz-timeout` to `nargo test` options
(noir-lang/noir#8326)
chore: bump external pinned commits
(noir-lang/noir#8334)
fix(expand): try to use "Self" in function calls
(noir-lang/noir#8353)
fix: Fix evaluation order of assignments with side-effects in their rhs
(noir-lang/noir#8342)
fix: let comptime Field value carry the field's sign
(noir-lang/noir#8343)
fix: Ordering of items in callstacks
(noir-lang/noir#8338)
chore: add snapshosts for nargo expand tests
(noir-lang/noir#8318)
fix(ownership): Clone global arrays
(noir-lang/noir#8328)
chore: Replace all SSA interpreter panics with error variants
(noir-lang/noir#8311)
feat: Metamorphic AST fuzzing
(noir-lang/noir#8299)
fix: fix some Display implementations for AST nodes
(noir-lang/noir#8316)
chore: remove leftover file
(noir-lang/noir#8313)
fix: uses non-zero points with ec-add-unsafe
(noir-lang/noir#8248)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: always type-check turbofish, and error when it's not allowed
(noir-lang/noir#8437)
chore: Release Noir(1.0.0-beta.5)
(noir-lang/noir#7955)
feat(greybox_fuzzer): Parallel fuzz tests
(noir-lang/noir#8432)
fix(ssa): Mislabeled instructions with side effects in
EnableSideEffectsIf removal pass
(noir-lang/noir#8355)
feat: SSA pass impact report
(noir-lang/noir#8393)
chore: bump external pinned commits
(noir-lang/noir#8433)
chore: separate benchmarking from github actions more
(noir-lang/noir#7943)
chore(fuzz): Break up the AST fuzzer `compare` module
(noir-lang/noir#8431)
chore(fuzz): Rename `init_vs_final` to `min_vs_full`
(noir-lang/noir#8430)
fix!: error on tuple mismatch
(noir-lang/noir#8424)
chore: bump external pinned commits
(noir-lang/noir#8429)
chore(acir): Test whether the predicate has an effect on slice
intrinsics (noir-lang/noir#8421)
feat(ssa): Mark transitively dead parameters during DIE
(noir-lang/noir#8254)
fix(ssa_gen): Do not code gen fetching of empty arrays when initializing
the data bus (noir-lang/noir#8426)
chore: remove `.aztec-sync-commit`
(noir-lang/noir#8415)
chore(test): Add more unit tests for
`inline_functions_with_at_most_one_instruction`
(noir-lang/noir#8418)
chore: add minor docs for interpreter
(noir-lang/noir#8397)
fix: print slice composite types surrounded by parentheses
(noir-lang/noir#8412)
feat: Skip SSA passes that contain any of the given messages
(noir-lang/noir#8416)
fix: disable range constraints using the predicate
(noir-lang/noir#8396)
chore: bumping external libraries
(noir-lang/noir#8406)
chore: redo typo PR by shystrui1199
(noir-lang/noir#8405)
feat(test): add `nargo_fuzz_target`
(noir-lang/noir#8308)
fix: allow names to collide in the values/types namespaces
(noir-lang/noir#8286)
fix: Fix sequencing of side-effects in lvalue
(noir-lang/noir#8384)
feat(greybox_fuzzer): Maximum executions parameter added
(noir-lang/noir#8390)
fix: warn on and discard unreachable statements after break and continue
(noir-lang/noir#8382)
fix: add handling for u128 infix ops in interpreter
(noir-lang/noir#8392)
chore: move acirgen tests into separate file
(noir-lang/noir#8376)
feat(fuzz): initial version of comptime vs brillig target for AST fuzzer
(noir-lang/noir#8335)
chore: apply lints to `ast_fuzzer`
(noir-lang/noir#8386)
chore: add note on AI generated PRs in `CONTRIBUTING.md`
(noir-lang/noir#8385)
chore: document flattening pass
(noir-lang/noir#8312)
fix: comptime shift-right overflow is zero
(noir-lang/noir#8380)
feat: let static_assert accept any type for its message
(noir-lang/noir#8322)
fix(expand): output safety comment before statements
(noir-lang/noir#8378)
chore: avoid need to rebuild after running tests
(noir-lang/noir#8379)
chore: bump dependencies (noir-lang/noir#8372)
chore: Add GITHUB_TOKEN to cross build
(noir-lang/noir#8370)
chore: redo typo PR by GarmashAlex
(noir-lang/noir#8364)
chore: remove unsafe code from greybox fuzzer
(noir-lang/noir#8315)
feat: add `--fuzz-timeout` to `nargo test` options
(noir-lang/noir#8326)
chore: bump external pinned commits
(noir-lang/noir#8334)
fix(expand): try to use "Self" in function calls
(noir-lang/noir#8353)
fix: Fix evaluation order of assignments with side-effects in their rhs
(noir-lang/noir#8342)
fix: let comptime Field value carry the field's sign
(noir-lang/noir#8343)
fix: Ordering of items in callstacks
(noir-lang/noir#8338)
chore: add snapshosts for nargo expand tests
(noir-lang/noir#8318)
fix(ownership): Clone global arrays
(noir-lang/noir#8328)
chore: Replace all SSA interpreter panics with error variants
(noir-lang/noir#8311)
feat: Metamorphic AST fuzzing
(noir-lang/noir#8299)
fix: fix some Display implementations for AST nodes
(noir-lang/noir#8316)
chore: remove leftover file
(noir-lang/noir#8313)
fix: uses non-zero points with ec-add-unsafe
(noir-lang/noir#8248)
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.

2 participants