Skip to content

fix(SSA): validate MakeArray instruction#9183

Merged
asterite merged 11 commits intomasterfrom
ab/ssa-validate-make-array
Jul 21, 2025
Merged

fix(SSA): validate MakeArray instruction#9183
asterite merged 11 commits intomasterfrom
ab/ssa-validate-make-array

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Jul 11, 2025

Blocked by #8918 as currently the databus puts values of the wrong type in MakeArray.

Description

Problem

Resolves #9158

Summary

With the validation in some multi_scalar_mul simplification tests started to fail. It turned out that some MakeArray types were incorrect there so this PR fixes that too.

Additional Context

While doing this I realized call argument types aren't checked against the target function so I might do that in a separate PR.

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 asterite added the bench-show Display benchmark results on PR label Jul 11, 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: e3dee5c Previous: 39a8379 Ratio
purely_sequential_opcodes 254903 ns/iter (± 1268) 250319 ns/iter (± 888) 1.02
perfectly_parallel_opcodes 223661 ns/iter (± 4685) 219732 ns/iter (± 1535) 1.02
perfectly_parallel_batch_inversion_opcodes 2781475 ns/iter (± 8723) 2781014 ns/iter (± 15137) 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.

Artifact Size

Details
Benchmark suite Current: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 716.6 KB 716.6 KB 1
private-kernel-reset 1972.3 KB 1972.3 KB 1
private-kernel-tail 543 KB 543 KB 1
rollup-base-private 4943 KB 4943 KB 1
rollup-base-public 3958.4 KB 3958.4 KB 1
rollup-block-root-empty 3881.2 KB 3881.2 KB 1
rollup-block-root-single-tx 32668.1 KB 32668.1 KB 1
rollup-block-root 32702.3 KB 32702.3 KB 1
rollup-merge 187.1 KB 187.1 KB 1
rollup-root 398.6 KB 398.6 KB 1
semaphore-depth-10 632.4 KB 632.4 KB 1
sha512-100-bytes 622.9 KB 622.9 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: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 2.074 s 1.964 s 1.06
private-kernel-reset 8.304 s 8.156 s 1.02
private-kernel-tail 1.63 s 1.612 s 1.01
rollup-base-private 17.56 s 17.86 s 0.98
rollup-base-public 14.76 s 15.02 s 0.98
rollup-block-root-empty 21.26 s 20.84 s 1.02
rollup-block-root-single-tx 198 s 213 s 0.93
rollup-block-root 206 s 199 s 1.04
rollup-merge 1.814 s 1.666 s 1.09
rollup-root 1.816 s 1.928 s 0.94
semaphore-depth-10 0.768 s 0.76 s 1.01
sha512-100-bytes 1.885 s 1.98 s 0.95

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: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 0.015 s 0.015 s 1
private-kernel-reset 0.154 s 0.152 s 1.01
private-kernel-tail 0.01 s 0.01 s 1
rollup-base-private 0.294 s 0.299 s 0.98
rollup-base-public 0.184 s 0.188 s 0.98
rollup-block-root 13.4 s 13.1 s 1.02
rollup-merge 0.002 s 0.002 s 1
rollup-root 0.007 s 0.004 s 1.75
semaphore-depth-10 0.019 s 0.019 s 1
sha512-100-bytes 0.086 s 0.055 s 1.56

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: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 235.34 MB 235.38 MB 1.00
private-kernel-reset 543.37 MB 543.37 MB 1
private-kernel-tail 209.04 MB 209.04 MB 1
rollup-base-private 1370 MB 1370 MB 1
rollup-base-public 1460 MB 1460 MB 1
rollup-block-root-empty 1080 MB 1080 MB 1
rollup-block-root-single-tx 9380 MB 9380 MB 1
rollup-block-root 9380 MB 9380 MB 1
rollup-merge 326.97 MB 326.97 MB 1
rollup-root 337.38 MB 337.38 MB 1
semaphore_depth_10 104.57 MB 104.57 MB 1
sha512_100_bytes 255.68 MB 255.76 MB 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 Memory

Details
Benchmark suite Current: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 202.08 MB 202.08 MB 1
private-kernel-reset 238.11 MB 238.11 MB 1
private-kernel-tail 191.3 MB 191.3 MB 1
rollup-base-private 500.49 MB 500.49 MB 1
rollup-base-public 433.35 MB 433.35 MB 1
rollup-block-root 1510 MB 1510 MB 1
rollup-merge 322.9 MB 322.9 MB 1
rollup-root 325.54 MB 325.54 MB 1
semaphore_depth_10 69.3 MB 69.3 MB 1
sha512_100_bytes 55.56 MB 55.56 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: e3dee5c Previous: 39a8379 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 131 s 134 s 0.98
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 153 s 149 s 1.03
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 177 s 178 s 0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 268 s 267 s 1.00
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib 34 s 34 s 1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 692 s 702 s 0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 128 s 128 s 1
test_report_noir-lang_noir-bignum_ 379 s 403 s 0.94
test_report_noir-lang_noir_bigcurve_ 363 s 320 s 1.13
test_report_noir-lang_sha512_ 16 s 16 s 1
test_report_zkpassport_noir-ecdsa_ 117 s 108 s 1.08
test_report_zkpassport_noir_rsa_ 1 s 1 s 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.

⚠️ 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: dd3ef76 Previous: 87aeccf Ratio
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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.

Opcode count

Details
Benchmark suite Current: e3dee5c Previous: 39a8379 Ratio
private-kernel-inner 15080 opcodes 15080 opcodes 1
private-kernel-reset 68865 opcodes 68865 opcodes 1
private-kernel-tail 11347 opcodes 11347 opcodes 1
rollup-base-private 261916 opcodes 261916 opcodes 1
rollup-base-public 201205 opcodes 201205 opcodes 1
rollup-block-root-empty 70748 opcodes 70748 opcodes 1
rollup-block-root-single-tx 1079444 opcodes 1079444 opcodes 1
rollup-block-root 1080739 opcodes 1080739 opcodes 1
rollup-merge 1419 opcodes 1419 opcodes 1
rollup-root 3136 opcodes 3136 opcodes 1
semaphore-depth-10 5763 opcodes 5763 opcodes 1
sha512-100-bytes 22169 opcodes 22169 opcodes 1

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 129f157eec725f857d0d0808aa844ac676192a69, compared to commit: 39a837906d40a728c0413f7ccc703df8e40f9716

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
embedded_curve_ops_inliner_max -28 ✅ -12.56%
embedded_curve_ops_inliner_zero -28 ✅ -12.56%

Full diff report 👇
Program Brillig opcodes (+/-) %
embedded_curve_ops_inliner_min 291 (-28) -8.78%
embedded_curve_ops_inliner_max 195 (-28) -12.56%
embedded_curve_ops_inliner_zero 195 (-28) -12.56%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2025

Changes to Brillig bytecode sizes

Generated at commit: 129f157eec725f857d0d0808aa844ac676192a69, compared to commit: 39a837906d40a728c0413f7ccc703df8e40f9716

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
embedded_curve_ops_inliner_max -142 ✅ -40.00%
embedded_curve_ops_inliner_zero -142 ✅ -40.00%

Full diff report 👇
Program Brillig opcodes (+/-) %
embedded_curve_ops_inliner_min 280 (-154) -35.48%
embedded_curve_ops_inliner_max 213 (-142) -40.00%
embedded_curve_ops_inliner_zero 213 (-142) -40.00%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2025

Changes to circuit sizes

Generated at commit: 129f157eec725f857d0d0808aa844ac676192a69, compared to commit: 39a837906d40a728c0413f7ccc703df8e40f9716

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
embedded_curve_ops -38 ✅ -74.51% -924 ✅ -14.60%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
embedded_curve_ops 13 (-38) -74.51% 5,404 (-924) -14.60%

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

Benchmark suite Current: dd3ef76 Previous: 8797651 Ratio
private-kernel-inner 2.458 s 2.024 s 1.21

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: e3dee5c Previous: 39a8379 Ratio
rollup-root 0.007 s 0.004 s 1.75
sha512-100-bytes 0.086 s 0.055 s 1.56

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

CC: @TomAFrench

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Great stuff 👏

@aakoshh
Copy link
Contributor

aakoshh commented Jul 17, 2025

Are the circuit size changes expected?

@asterite
Copy link
Collaborator Author

Are the circuit size changes expected?

Good question. I hope so! 😄

@asterite
Copy link
Collaborator Author

Are the circuit size changes expected?

The multi_scalar_multiplication simplification was returning incorrect types. For example instead of [(Field, Field, u1); N] it was returning [Field; 6] which I guess later on has some different implications compared to the correct type. But... I'll compare the SSAs to see what changed to understand this more.

@asterite
Copy link
Collaborator Author

I just checked the SSA difference and it seems now we can reuse existing arrays. Previously they would get an incorrect type after simplification so they didn't match previous arrays. Or at least that's one difference...

@asterite asterite enabled auto-merge July 17, 2025 19:51
@asterite asterite disabled auto-merge July 17, 2025 19:51
@asterite
Copy link
Collaborator Author

@TomAFrench Could you check if this change is okay? Mainly to make sure the performance improvements are correct and that I didn't break anything 😅

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.

I looked over the improvements and as you mentioned they look to be happening as simplifications revealed themselves once we accurately marked the array types. When looking at things to optimize in the past I was always thinking the embedded_curve_add was more expensive than expected. I didn't investigate too deeply where the exact simplification occurs that has knockdown benefits, but the types you marked look correct.

@asterite asterite enabled auto-merge July 21, 2025 17:44
@asterite asterite added this pull request to the merge queue Jul 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 21, 2025
@asterite asterite added this pull request to the merge queue Jul 21, 2025
Merged via the queue into master with commit 366149f Jul 21, 2025
121 of 122 checks passed
@asterite asterite deleted the ab/ssa-validate-make-array branch July 21, 2025 18:51
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(SSA): validate MakeArray instruction
(noir-lang/noir#9183)
chore(docs): Add links to ACIR and source reference docs
(noir-lang/noir#9260)
fix: comptime code not mutating shared ref to struct field
(noir-lang/noir#9250)
fix(acir_gen): Bail out of `handle_constant_index` when it encounters
`DynamicArray` (noir-lang/noir#9259)
feat: allow paths in l-values
(noir-lang/noir#9254)
fix: parse AsTraitPath in type expressions
(noir-lang/noir#9258)
chore: add acir-gen unit tests per ssa instruction (2)
(noir-lang/noir#9185)
fix(licm): Ensure that all nested loops the current block is part of are
guaranteed to execute (noir-lang/noir#9249)
chore: bump external pinned commits
(noir-lang/noir#9256)
chore: enforce clippy in `ssa_fuzzer`
(noir-lang/noir#9247)
chore: clippy (noir-lang/noir#9246)
chore: skip `ram_blowup_regression` on PRs
(noir-lang/noir#9231)
chore: mark bignum as expected to pass
(noir-lang/noir#9244)
fix: suggest traits via visible reexports if they are not directly
visible (noir-lang/noir#9242)
fix: bind self when type-checking AsTraitPath
(noir-lang/noir#9236)
chore(docs): Include list to hashing libraries at the top of the
relevant docs page (noir-lang/noir#9239)
fix(fuzz): Use scoping for variable dynamism
(noir-lang/noir#9233)
fix(ssa): Change constraint message to "multiply"
(noir-lang/noir#9230)
feat: Add `compiler_unstable_features` to `Nargo.toml`
(noir-lang/noir#9219)
chore(fuzz): Increase loop frequency in Brillig
(noir-lang/noir#9228)
chore: bump noir-edwards dep
(noir-lang/noir#9229)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(SSA): validate MakeArray instruction
(noir-lang/noir#9183)
chore(docs): Add links to ACIR and source reference docs
(noir-lang/noir#9260)
fix: comptime code not mutating shared ref to struct field
(noir-lang/noir#9250)
fix(acir_gen): Bail out of `handle_constant_index` when it encounters
`DynamicArray` (noir-lang/noir#9259)
feat: allow paths in l-values
(noir-lang/noir#9254)
fix: parse AsTraitPath in type expressions
(noir-lang/noir#9258)
chore: add acir-gen unit tests per ssa instruction (2)
(noir-lang/noir#9185)
fix(licm): Ensure that all nested loops the current block is part of are
guaranteed to execute (noir-lang/noir#9249)
chore: bump external pinned commits
(noir-lang/noir#9256)
chore: enforce clippy in `ssa_fuzzer`
(noir-lang/noir#9247)
chore: clippy (noir-lang/noir#9246)
chore: skip `ram_blowup_regression` on PRs
(noir-lang/noir#9231)
chore: mark bignum as expected to pass
(noir-lang/noir#9244)
fix: suggest traits via visible reexports if they are not directly
visible (noir-lang/noir#9242)
fix: bind self when type-checking AsTraitPath
(noir-lang/noir#9236)
chore(docs): Include list to hashing libraries at the top of the
relevant docs page (noir-lang/noir#9239)
fix(fuzz): Use scoping for variable dynamism
(noir-lang/noir#9233)
fix(ssa): Change constraint message to "multiply"
(noir-lang/noir#9230)
feat: Add `compiler_unstable_features` to `Nargo.toml`
(noir-lang/noir#9219)
chore(fuzz): Increase loop frequency in Brillig
(noir-lang/noir#9228)
chore: bump noir-edwards dep
(noir-lang/noir#9229)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Jan Beneš <janbenes1234@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.

ACIR vs Brillig; array of bigger size than defined

3 participants