Skip to content

fix: always use u32 for indexing arrays in SSA#8633

Merged
asterite merged 4 commits intomasterfrom
ab/disallow-non-u32-index-in-ssa-array-get-set
May 23, 2025
Merged

fix: always use u32 for indexing arrays in SSA#8633
asterite merged 4 commits intomasterfrom
ab/disallow-non-u32-index-in-ssa-array-get-set

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves #8611

Summary

Additional Context

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.

@asterite
Copy link
Collaborator Author

@aakoshh In this PR there are two failing tests now:

  • ssa::opt::unrolling::tests::test_boilerplate_stats_i64_empty
  • ssa::opt::unrolling::tests::test_boilerplate_stats_i64_non_empty

They are missing casting the index to u32. But when I try to do that the stats change. I change the numbers but then the loop says it's not small anymore and I'm not sure how should the test be changed to make it pass (and still make it test what it should be testing).

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: 3f5cf05 Previous: 23d7409 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 4 s 3 s 1.33

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

CC: @TomAFrench

@asterite asterite added the bench-show Display benchmark results on PR label May 22, 2025
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.

ACVM Benchmarks

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
purely_sequential_opcodes 255308 ns/iter (± 838) 254134 ns/iter (± 1158) 1.00
perfectly_parallel_opcodes 225547 ns/iter (± 4880) 224090 ns/iter (± 2199) 1.01
perfectly_parallel_batch_inversion_opcodes 3212077 ns/iter (± 12496) 3213857 ns/iter (± 21235) 1.00

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

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.

Execution Time

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
private-kernel-inner 0.028 s 0.028 s 1
private-kernel-reset 0.162 s 0.162 s 1
private-kernel-tail 0.012 s 0.011 s 1.09
rollup-base-private 0.308 s 0.308 s 1
rollup-base-public 0.198 s 0.203 s 0.98
rollup-block-root 11.3 s 11.5 s 0.98
rollup-merge 0.004 s 0.004 s 1
rollup-root 0.013 s 0.012 s 1.08
semaphore-depth-10 0.021 s 0.024 s 0.88

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

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.

Artifact Size

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
private-kernel-inner 1130 KB 1130 KB 1
private-kernel-reset 2051 KB 2051 KB 1
private-kernel-tail 583.9 KB 583.9 KB 1
rollup-base-private 5130.1 KB 5130.1 KB 1
rollup-base-public 4067.2 KB 4067.2 KB 1
rollup-block-root-empty 256.7 KB 256.7 KB 1
rollup-block-root-single-tx 25697.4 KB 25697.4 KB 1
rollup-block-root 25720.7 KB 25720.7 KB 1
rollup-merge 181.6 KB 181.6 KB 1
rollup-root 478.9 KB 478.9 KB 1
semaphore-depth-10 636.3 KB 636.3 KB 1

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

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.

Compilation Time

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
private-kernel-inner 2.5 s 2.374 s 1.05
private-kernel-reset 6.636 s 6.75 s 0.98
private-kernel-tail 1.202 s 1.08 s 1.11
rollup-base-private 16.5 s 16.34 s 1.01
rollup-base-public 13.98 s 13.96 s 1.00
rollup-block-root-empty 1.376 s 1.298 s 1.06
rollup-block-root-single-tx 126 s 124 s 1.02
rollup-block-root 132 s 136 s 0.97
rollup-merge 1.062 s 1.128 s 0.94
rollup-root 1.666 s 1.68 s 0.99
semaphore-depth-10 0.902 s 0.873 s 1.03

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

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.

Execution Memory

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
private-kernel-inner 241.49 MB 241.49 MB 1
private-kernel-reset 263.53 MB 263.53 MB 1
private-kernel-tail 216.67 MB 216.67 MB 1
rollup-base-private 548.28 MB 548.28 MB 1
rollup-base-public 544.36 MB 544.36 MB 1
rollup-block-root 1440 MB 1440 MB 1
rollup-merge 370.11 MB 370.11 MB 1
rollup-root 376.12 MB 376.12 MB 1
semaphore_depth_10 92.93 MB 92.93 MB 1

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

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.

Compilation Memory

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
private-kernel-inner 332.52 MB 332.54 MB 1.00
private-kernel-reset 571.81 MB 571.81 MB 1
private-kernel-tail 232.33 MB 232.34 MB 1.00
rollup-base-private 1440 MB 1440 MB 1
rollup-base-public 1600 MB 1600 MB 1
rollup-block-root-empty 399.93 MB 399.92 MB 1.00
rollup-block-root-single-tx 7210 MB 7210 MB 1
rollup-block-root 7220 MB 7220 MB 1
rollup-merge 384.2 MB 384.19 MB 1.00
rollup-root 446.3 MB 446.31 MB 1.00
semaphore_depth_10 128.37 MB 128.37 MB 1

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

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.

Test Suite Duration

Details
Benchmark suite Current: 1b6cb8d Previous: 08505ab Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 65 s 65 s 1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 95 s 104 s 0.91
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 47 s 41 s 1.15
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 189 s 191 s 0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 182 s 187 s 0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 63 s 60 s 1.05
test_report_noir-lang_noir-bignum_ 379 s 374 s 1.01
test_report_noir-lang_noir_bigcurve_ 220 s 227 s 0.97
test_report_noir-lang_sha512_ 30 s 31 s 0.97
test_report_zkpassport_noir_rsa_ 2 s 2 s 1

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

@aakoshh
Copy link
Contributor

aakoshh commented May 22, 2025

@asterite the tests are asserting the following:

test_boilerplate_stats_i64_empty

This checks that we will have 0 iterations when the SSA looks like this:

   jmp b1(i64 0)
          b1(v1: i64):
            v7 = lt v1, i64 {u64::MAX}

whatever the instructions turn into, this should be 0 iterations, because it goes for i in 0..-1.

test_boilerplate_stats_i64_non_empty

This one should achieve 3 iterations, and that's the only thing it seems to assert, so even if you need to add more instructions, it shouldn't fail 🤔

I can imagine the ones with is_small() failing if the number of instructions increased. I think the only way to keep them small is to reduce the number of iterations.

@asterite
Copy link
Collaborator Author

I think the only way to keep them small is to reduce the number of iterations.

Thank you, that worked!

@asterite asterite requested a review from a team May 22, 2025 20:39
@asterite asterite added this pull request to the merge queue May 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 23, 2025
@asterite asterite added this pull request to the merge queue May 23, 2025
Merged via the queue into master with commit a16e848 May 23, 2025
117 of 118 checks passed
@asterite asterite deleted the ab/disallow-non-u32-index-in-ssa-array-get-set branch May 23, 2025 17:41
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(fmt): correctly format mixed secondary attributes and doc comments
(noir-lang/noir#8735)
fix!: require types for trait impl associated constants
(noir-lang/noir#8734)
fix!: Prevent returning references from if expressions
(noir-lang/noir#8731)
fix: cast signed to u1 follow-up
(noir-lang/noir#8730)
fix: cast signed to u1 (noir-lang/noir#8720)
fix: do not mutate arrays later copied inside other arrays
(noir-lang/noir#8701)
chore: box `AsTraitPath` and `TypePath` in `ExpressionKind`
(noir-lang/noir#8716)
fix: general solution for accessing associated constants
(noir-lang/noir#8417)
fix(ssa): Validate checked signed add/sub is followed by a truncate
(noir-lang/noir#8706)
chore: add pre- and post-check on `array_set` optimization pass
(noir-lang/noir#8714)
fix: merge expr bindings with instantiations bindings during
monomorphization (noir-lang/noir#8713)
fix: better way to do LSP file overrides
(noir-lang/noir#8702)
fix(fmt): correct indentation when formatting long struct patterns
(noir-lang/noir#8711)
fix(fuzz): Prevent breaking/continuing out from let blocks in the AST
fuzzer (noir-lang/noir#8708)
chore: remove override for zero length inputs
(noir-lang/noir#8709)
feat: Replace converging jmpif with empty block destinations with a
single jmp (noir-lang/noir#8613)
feat(cli): Add `nargo interpret` command
(noir-lang/noir#8700)
chore: more 1-tuple printing fixes
(noir-lang/noir#8699)
chore(fuzz): Fuzz the SSA parser
(noir-lang/noir#8647)
fix: (SSA parser) translate blocks in logical order rather than syntax
order (noir-lang/noir#8668)
fix(SSA): show and parse range_check's assert_message
(noir-lang/noir#8652)
chore: Show the step number in the SSA message
(noir-lang/noir#8698)
chore(docs): Add pointers to tests
(noir-lang/noir#8695)
chore(fuzz): Capture comptime print output
(noir-lang/noir#8635)
fix: nargo expand reexports correctly implemented
(noir-lang/noir#8693)
feat(cli): Show multiple SSA passes
(noir-lang/noir#8692)
chore(test): Add regression test for defunctionalize
(noir-lang/noir#8691)
chore: add an assertion when parsing SSA that all functions are
well-formed (noir-lang/noir#8671)
feat!: prevent compiling blake3 hashes which barretenberg cannot prove
(noir-lang/noir#8690)
fix(ssa): Remove the array cache from the function inserter
(noir-lang/noir#8607)
chore: bump external pinned commits
(noir-lang/noir#8684)
chore: reenable fuzzing tests in CI
(noir-lang/noir#8688)
fix: Handle `&mut function` in defunctionalize
(noir-lang/noir#8665)
fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and
HOFs in arrays (noir-lang/noir#8672)
chore(ci): avoid running fuzzer tests in the merge queue
(noir-lang/noir#8664)
fix: Make casts in `comptime` consistent with runtime casts
(noir-lang/noir#8669)
fix: relax connectedness requirement on purity analysis pass
(noir-lang/noir#8667)
fix: always use `u32` for indexing arrays in SSA
(noir-lang/noir#8633)
chore: explicitly pull from `next` branch in aztec-packages
(noir-lang/noir#8660)
chore(fuzz): Build the dictionary from the SSA
(noir-lang/noir#8591)
chore(docs): Remove old versioned docs
(noir-lang/noir#8061)
chore: bump external pinned commits
(noir-lang/noir#8634)
fix(SSA): don't use string literal if byte is "form feed" ('\f')
(noir-lang/noir#8653)
chore(defunctionalize): Add regression test for missing lambda variants
panic (noir-lang/noir#8642)
chore(ci): Do not run ast_fuzzer orig vs. morph in ci
(noir-lang/noir#8646)
fix: disable underflow fix for fields
(noir-lang/noir#8631)
fix: revert "fix: error on unused generic in trait impl
(noir-lang/noir#8395)"
(noir-lang/noir#8636)
fix(ssa interpreter): Default to zero when we have an overflowing shl
(noir-lang/noir#8638)
fix: ensure that purity analysis pass explores all functions
(noir-lang/noir#8452)
feat: acir_formal_proofs (noir-lang/noir#8140)
fix: error on unused generic in trait impl
(noir-lang/noir#8395)
fix(expand): use re-exports for non-visibile items
(noir-lang/noir#8374)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bench-show Display benchmark results on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSA Interpreter: Expected u32 value in array get index but instead found Field (encore)

4 participants