Skip to content

feat(SSA): simplify array_get on slice_insert#10295

Closed
asterite wants to merge 14 commits intomasterfrom
ab/simplify-array-get-on-slice-insert
Closed

feat(SSA): simplify array_get on slice_insert#10295
asterite wants to merge 14 commits intomasterfrom
ab/simplify-array-get-on-slice-insert

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves #10278

Summary

Alternative to #10279 with what's written in #10279 (comment)

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.

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

Benchmark suite Current: 0022fa7 Previous: 52b341d Ratio
perfectly_parallel_batch_inversion_opcodes 2794287 ns/iter (± 1635) 2259833 ns/iter (± 14138) 1.24

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

CC: @TomAFrench

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

Changes to Brillig bytecode sizes

Generated at commit: 8419bde49732845b160b437b9f6b4b9873f790da, compared to commit: b43153bd34f52ea835f6b7715bcf2c1f931a0fc8

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_arrays_inliner_min -40 ✅ -29.63%
brillig_arrays_inliner_max -40 ✅ -35.40%
brillig_arrays_inliner_zero -40 ✅ -35.40%

Full diff report 👇
Program Brillig opcodes (+/-) %
slices_inliner_min 2,176 (-4) -0.18%
slices_inliner_max 1,669 (-16) -0.95%
slices_inliner_zero 1,629 (-16) -0.97%
slice_dynamic_insert_inliner_max 378 (-4) -1.05%
slice_dynamic_insert_inliner_zero 378 (-4) -1.05%
regression_capacity_tracker_inliner_max 212 (-3) -1.40%
regression_capacity_tracker_inliner_min 212 (-3) -1.40%
regression_capacity_tracker_inliner_zero 212 (-3) -1.40%
array_to_slice_inliner_max 482 (-9) -1.83%
slice_dynamic_insert_inliner_min 425 (-8) -1.85%
slice_dynamic_index_inliner_min 2,102 (-48) -2.23%
slice_dynamic_index_inliner_max 1,553 (-73) -4.49%
slice_dynamic_index_inliner_zero 1,553 (-73) -4.49%
brillig_arrays_inliner_min 95 (-40) -29.63%
brillig_arrays_inliner_max 73 (-40) -35.40%
brillig_arrays_inliner_zero 73 (-40) -35.40%

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 8419bde49732845b160b437b9f6b4b9873f790da, compared to commit: b43153bd34f52ea835f6b7715bcf2c1f931a0fc8

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_arrays_inliner_min -57 ✅ -34.34%
brillig_arrays_inliner_max -57 ✅ -41.30%
brillig_arrays_inliner_zero -57 ✅ -41.30%

Full diff report 👇
Program Brillig opcodes (+/-) %
slices_inliner_min 3,797 (-2) -0.05%
slice_dynamic_insert_inliner_max 951 (-2) -0.21%
slice_dynamic_insert_inliner_zero 951 (-2) -0.21%
regression_capacity_tracker_inliner_max 566 (-2) -0.35%
regression_capacity_tracker_inliner_min 566 (-2) -0.35%
regression_capacity_tracker_inliner_zero 566 (-2) -0.35%
slices_inliner_zero 2,720 (-10) -0.37%
slice_dynamic_insert_inliner_min 1,061 (-4) -0.38%
slices_inliner_max 2,522 (-10) -0.39%
slice_dynamic_index_inliner_min 4,541 (-26) -0.57%
array_to_slice_inliner_max 942 (-6) -0.63%
slice_dynamic_index_inliner_max 3,543 (-41) -1.14%
slice_dynamic_index_inliner_zero 3,543 (-41) -1.14%
brillig_arrays_inliner_min 109 (-57) -34.34%
brillig_arrays_inliner_max 81 (-57) -41.30%
brillig_arrays_inliner_zero 81 (-57) -41.30%

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

Changes to circuit sizes

Generated at commit: 8419bde49732845b160b437b9f6b4b9873f790da, compared to commit: b43153bd34f52ea835f6b7715bcf2c1f931a0fc8

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_9329 -17 ✅ -45.95% -14 ✅ -10.53%
slice_dynamic_insert -33 ✅ -84.62% -119 ✅ -66.48%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
conditional_1 2,618 (-12) -0.46% 17,416 (0) 0.00%
regression_5252 23,283 (-1,008) -4.15% 72,270 (-3,810) -5.01%
array_if_cond_simple 32 (-8) -20.00% 2,851 (-213) -6.95%
regression_9329 20 (-17) -45.95% 119 (-14) -10.53%
slice_dynamic_insert 6 (-33) -84.62% 60 (-119) -66.48%

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: f232806 Previous: b43153b Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 2 s 1.50

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: 957da15 Previous: b43153b Ratio
rollup-block-root-first-empty-tx 0.004 s 0.003 s 1.33

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

CC: @TomAFrench

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 do think these optimizations make sense. They are not super invasive so I would be ok with adding them (especially the parameter array change in try_optimize_array_get_from_previous_set).

Comment on lines +75 to +82
if let Some(length) = context.dfg.get_numeric_constant(arguments[0]) {
// For `slice_insert(length, ...)` we can replace the resulting length with length + 1
let length = length + FieldElement::one();
let new_slice_length = context.dfg.instruction_results(instruction_id)[0];
Some((new_slice_length, length))
} else {
None
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if let Some(length) = context.dfg.get_numeric_constant(arguments[0]) {
// For `slice_insert(length, ...)` we can replace the resulting length with length + 1
let length = length + FieldElement::one();
let new_slice_length = context.dfg.instruction_results(instruction_id)[0];
Some((new_slice_length, length))
} else {
None
}
// For `slice_insert(length, ...)` we can replace the resulting length with length + 1
context.dfg.get_numeric_constant(arguments[0]).map(|length| {
let length = length + FieldElement::one();
let new_slice_length = context.dfg.instruction_results(instruction_id)[0];
(new_slice_length, length)
})

nit: option for reducing nesting

None
}
} else if slice_remove.is_some_and(|op| target_func == &op) {
if let Some(length) = context.dfg.get_numeric_constant(arguments[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

///
/// Note that this pass must be placed before loop unrolling to be useful.
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) fn slice_instrinsics_length_optimization(mut self) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub(crate) fn slice_instrinsics_length_optimization(mut self) -> Self {
pub(crate) fn slice_intrinsics_length_optimization(mut self) -> Self {

return SimplifyResult::None;
}

// slice_insert(length, slice, index, values...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this still?

acir(inline) fn main f0 {
b0():
v0 = make_array [Field 2, Field 3] : [Field]
v1, v2 = call slice_insert(u32 2, v0, u32 1, Field 4) -> (u32, [Field])
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add a test for the negative case that when we have a dynamic value for the length but a constant array we do not do anything.

@asterite
Copy link
Collaborator Author

I do think these optimizations make sense. They are not super invasive so I would be ok with adding them (especially the parameter array change in try_optimize_array_get_from_previous_set).

Cool! I just read your comment, I kept pushing changes with more "slice intrinsics length" optimizations.

If it's fine with you, I think I'd prefer to split this PR into smaller PR, each with one of the proposed optimizations, just so that if there's an issue with just one of them it's easier to revert, etc.

Now I'm thinking optimizing array_get on slice_insert is good, but we can also optimize array_get on slice_remove, slice_push_back, etc., as long as we know the slice length at compile-time. Maybe this is just another variant not considered in try_optimize_array_set_from_previous_get (though the index needs to be adjusted accordingly).

I might also introduce a new SSA pass instead of changing as_slice_length because we recently audited it so I'd feel bad modifying it 😅 (though the pass is pretty simple...)

@asterite asterite added the bench-show Display benchmark results on PR label Oct 27, 2025
@vezenovm
Copy link
Contributor

If it's fine with you, I think I'd prefer to split this PR into smaller PR, each with one of the proposed optimizations, just so that if there's an issue with just one of them it's easier to revert, etc.

Yeah I think that would be a good idea.

I might also introduce a new SSA pass instead of changing as_slice_length because we recently audited it so I'd feel bad modifying it 😅 (though the pass is pretty simple...)

Whatever you think is best here. As you said the actual as_slice opt logic is very minimal, but it does increase our audit scope so perhaps we should just contain it all in a single pass. Especially if we are able to later remove the as_slice_length opt if we remove the as_slice intrinsic as per #10279 (comment).

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: 0106d13 Previous: b43153b Ratio
purely_sequential_opcodes 262673 ns/iter (± 5426) 263529 ns/iter (± 508) 1.00
perfectly_parallel_opcodes 231729 ns/iter (± 6656) 233286 ns/iter (± 2788) 0.99
perfectly_parallel_batch_inversion_opcodes 2796221 ns/iter (± 1920) 2797292 ns/iter (± 14705) 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.

Test Suite Duration

Details
Benchmark suite Current: 0106d13 Previous: b43153b Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 121 s 116 s 1.04
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 131 s 134 s 0.98
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 254 s 273 s 0.93
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 242 s 233 s 1.04
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 127 s 126 s 1.01
test_report_noir-lang_noir-bignum_ 170 s 152 s 1.12
test_report_noir-lang_noir_bigcurve_ 357 s 378 s 0.94
test_report_noir-lang_sha256_ 14 s 15 s 0.93
test_report_noir-lang_sha512_ 14 s 14 s 1
test_report_zkpassport_noir-ecdsa_ 2 s 2 s 1
test_report_zkpassport_noir_rsa_ 2 s 2 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.

Execution Time

Details
Benchmark suite Current: 0106d13 Previous: b43153b Ratio
private-kernel-inner 0.011 s 0.011 s 1
private-kernel-reset 0.146 s 0.147 s 0.99
private-kernel-tail 0.009 s 0.009 s 1
rollup-block-root-first-empty-tx 0.003 s 0.003 s 1
rollup-block-root-single-tx 0.003 s 0.003 s 1
rollup-block-root 0.004 s 0.004 s 1
rollup-checkpoint-merge 0.003 s 0.003 s 1
rollup-checkpoint-root-single-block 10.9 s 11.4 s 0.96
rollup-checkpoint-root 10.9 s 11.2 s 0.97
rollup-root 0.004 s 0.004 s 1
rollup-tx-base-private 0.302 s 0.297 s 1.02
rollup-tx-base-public 0.234 s 0.244 s 0.96
rollup-tx-merge 0.002 s 0.002 s 1
semaphore-depth-10 0.008 s 0.009 s 0.89
sha512-100-bytes 0.054 s 0.061 s 0.89

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.

Opcode count

Details
Benchmark suite Current: 0106d13 Previous: b43153b Ratio
private-kernel-inner 14544 opcodes 14544 opcodes 1
private-kernel-reset 70415 opcodes 70415 opcodes 1
private-kernel-tail 11680 opcodes 11680 opcodes 1
rollup-block-root-first-empty-tx 1348 opcodes 1364 opcodes 0.99
rollup-block-root-single-tx 1033 opcodes 1048 opcodes 0.99
rollup-block-root 2409 opcodes 2409 opcodes 1
rollup-checkpoint-merge 2130 opcodes 2130 opcodes 1
rollup-checkpoint-root-single-block 962003 opcodes 962015 opcodes 1.00
rollup-checkpoint-root 963375 opcodes 963375 opcodes 1
rollup-root 2630 opcodes 2630 opcodes 1
rollup-tx-base-private 263892 opcodes 263908 opcodes 1.00
rollup-tx-base-public 245168 opcodes 245185 opcodes 1.00
rollup-tx-merge 1486 opcodes 1486 opcodes 1
semaphore-depth-10 5699 opcodes 5699 opcodes 1
sha512-100-bytes 13173 opcodes 13173 opcodes 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: 0106d13 Previous: b43153b Ratio
private-kernel-inner 1.748 s 1.812 s 0.96
private-kernel-reset 7.194 s 7.378 s 0.98
private-kernel-tail 1.39 s 1.334 s 1.04
rollup-block-root-first-empty-tx 1.364 s 1.406 s 0.97
rollup-block-root-single-tx 1.43 s 1.46 s 0.98
rollup-block-root 1.48 s 1.47 s 1.01
rollup-checkpoint-merge 1.452 s 1.488 s 0.98
rollup-checkpoint-root-single-block 214 s 202 s 1.06
rollup-checkpoint-root 193 s 214 s 0.90
rollup-root 1.52 s 1.552 s 0.98
rollup-tx-base-private 19.58 s 18.98 s 1.03
rollup-tx-base-public 81.24 s 81.74 s 0.99
rollup-tx-merge 1.352 s 1.394 s 0.97
semaphore-depth-10 0.784 s 0.793 s 0.99
sha512-100-bytes 1.61 s 2.008 s 0.80

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: 0106d13 Previous: b43153b Ratio
private-kernel-inner 714.3 KB 714.3 KB 1
private-kernel-reset 1864 KB 1864 KB 1
private-kernel-tail 546.2 KB 546.2 KB 1
rollup-block-root-first-empty-tx 179.2 KB 179.6 KB 1.00
rollup-block-root-single-tx 177.6 KB 177.9 KB 1.00
rollup-block-root 257.9 KB 257.9 KB 1
rollup-checkpoint-merge 370.6 KB 370.6 KB 1
rollup-checkpoint-root-single-block 27638.6 KB 27640.2 KB 1.00
rollup-checkpoint-root 27685.9 KB 27685.9 KB 1
rollup-root 411.4 KB 411.4 KB 1
rollup-tx-base-private 4906.5 KB 4909.3 KB 1.00
rollup-tx-base-public 4554.4 KB 4555.4 KB 1.00
rollup-tx-merge 186.1 KB 186.1 KB 1
semaphore-depth-10 570.9 KB 570.9 KB 1
sha512-100-bytes 506.3 KB 506.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 Memory

Details
Benchmark suite Current: 0106d13 Previous: b43153b Ratio
private-kernel-inner 254.47 MB 254.47 MB 1
private-kernel-reset 493.18 MB 493.18 MB 1
private-kernel-tail 244.63 MB 244.63 MB 1
rollup-block-root-first-empty-tx 339.37 MB 339.38 MB 1.00
rollup-block-root-single-tx 337.41 MB 337.41 MB 1
rollup-block-root 340.43 MB 340.43 MB 1
rollup-checkpoint-merge 339.76 MB 339.76 MB 1
rollup-checkpoint-root-single-block 5910 MB 5910 MB 1
rollup-checkpoint-root 5920 MB 5920 MB 1
rollup-root 341.31 MB 341.31 MB 1
rollup-tx-base-private 1070 MB 1070 MB 1
rollup-tx-base-public 2890 MB 2890 MB 1
rollup-tx-merge 336.59 MB 336.59 MB 1
semaphore_depth_10 92.18 MB 92.18 MB 1
sha512_100_bytes 185.48 MB 185.48 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.

Execution Memory

Details
Benchmark suite Current: 0106d13 Previous: b43153b Ratio
private-kernel-inner 253.63 MB 253.63 MB 1
private-kernel-reset 287.24 MB 287.24 MB 1
private-kernel-tail 243.5 MB 243.5 MB 1
rollup-block-root 337.87 MB 337.87 MB 1
rollup-checkpoint-merge 336.67 MB 336.67 MB 1
rollup-checkpoint-root-single-block 1020 MB 1020 MB 1
rollup-checkpoint-root 1020 MB 1020 MB 1
rollup-root 337.87 MB 337.87 MB 1
rollup-tx-base-private 451.41 MB 451.42 MB 1.00
rollup-tx-base-public 466.49 MB 466.49 MB 1
rollup-tx-merge 336.13 MB 336.13 MB 1
semaphore_depth_10 73.7 MB 73.7 MB 1
sha512_100_bytes 71.96 MB 71.96 MB 1

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

@asterite
Copy link
Collaborator Author

Closing in favor of #10300 and other upcoming PRs.

@asterite asterite closed this Oct 27, 2025
@asterite asterite deleted the ab/simplify-array-get-on-slice-insert branch October 27, 2025 18:39
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.

as_slice could be simplified in more cases

2 participants