fix: add offset to ArrayGet

[asterite]

Description

Problem

Resolves #8462

Summary

I thought about adding a boolean flag to ArrayGet to track whether the index has been offset or not. That would have worked as well, but then in the interpreter we'd have to check if the target is an array or a slice, then use 1 or 3, duplicating this value. It's not a big duplication, but imagine the internal representation in brillig changes, we'd have to remember to change these in two places. I guess dedicated constants would have worked well too.

With the current approach the offset is tracked by an enum that knows if it's 1 or 3. Then these values should up in the SSA text representation:

v2 = array_get v0, index u32 10 minus 3 -> Field

making it clearer that the actual index is 10 - 3 = 7. I chose "minus" here because SSA is very textual, there are no operators.

But let me know if I should change this to a bool.

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.

[github-actions]

ACVM Benchmarks

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
purely_sequential_opcodes	262093 ns/iter (± 1778)	263476 ns/iter (± 822)	0.99
perfectly_parallel_opcodes	231393 ns/iter (± 3556)	232879 ns/iter (± 3843)	0.99
perfectly_parallel_batch_inversion_opcodes	3572130 ns/iter (± 13684)	3572682 ns/iter (± 1583)	1.00

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

[github-actions]

Artifact Size

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
private-kernel-inner	1127.1 KB	1127.1 KB	1
private-kernel-reset	2051 KB	2051 KB	1
private-kernel-tail	583.1 KB	583.1 KB	1
rollup-base-private	5123.2 KB	5123.2 KB	1
rollup-base-public	3941.9 KB	3941.9 KB	1
rollup-block-root-empty	256.8 KB	256.8 KB	1
rollup-block-root-single-tx	25679 KB	25679 KB	1
rollup-block-root	25706.2 KB	25706.2 KB	1
rollup-merge	181.7 KB	181.7 KB	1
rollup-root	473.4 KB	473.4 KB	1
semaphore-depth-10	636.4 KB	636.4 KB	1

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

[github-actions]

Execution Time

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
private-kernel-inner	0.028 s	0.028 s	1
private-kernel-reset	0.159 s	0.16 s	0.99
private-kernel-tail	0.012 s	0.011 s	1.09
rollup-base-private	0.304 s	0.302 s	1.01
rollup-base-public	0.195 s	0.198 s	0.98
rollup-block-root	11.2 s	11.2 s	1
rollup-merge	0.004 s	0.004 s	1
rollup-root	0.013 s	0.013 s	1
semaphore-depth-10	0.019 s	0.02 s	0.95

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

[github-actions]

Compilation Time

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
private-kernel-inner	2.536 s	2.372 s	1.07
private-kernel-reset	6.688 s	6.424 s	1.04
private-kernel-tail	1.134 s	1.154 s	0.98
rollup-base-private	16.42 s	15.84 s	1.04
rollup-base-public	13.08 s	14.78 s	0.88
rollup-block-root-empty	1.31 s	1.314 s	1.00
rollup-block-root-single-tx	124 s	128 s	0.97
rollup-block-root	125 s	127 s	0.98
rollup-merge	1.096 s	1.092 s	1.00
rollup-root	1.674 s	1.73 s	0.97
semaphore-depth-10	0.832 s	0.842 s	0.99

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

[github-actions]

Test Suite Duration

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	66 s	66 s	1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	102 s	100 s	1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	43 s	47 s	0.91
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	191 s	193 s	0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	185 s	182 s	1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	60 s	62 s	0.97
test_report_noir-lang_noir-bignum_	450 s	376 s	1.20
test_report_noir-lang_noir_bigcurve_	242 s	230 s	1.05
test_report_noir-lang_sha512_	31 s	30 s	1.03
test_report_zkpassport_noir_rsa_	23 s	25 s	0.92

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

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
private-kernel-inner	313.29 MB	313.29 MB	1
private-kernel-reset	569.09 MB	569.09 MB	1
private-kernel-tail	228.21 MB	228.22 MB	1.00
rollup-base-private	1440 MB	1440 MB	1
rollup-base-public	1460 MB	1460 MB	1
rollup-block-root-empty	398.7 MB	398.65 MB	1.00
rollup-block-root-single-tx	7210 MB	7210 MB	1
rollup-block-root	7220 MB	7220 MB	1
rollup-merge	380.72 MB	380.74 MB	1.00
rollup-root	441.65 MB	441.66 MB	1.00
semaphore_depth_10	126.94 MB	126.94 MB	1

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

[github-actions]

Execution Memory

Details

Benchmark suite	Current: c3ece69	Previous: e2a52b7	Ratio
private-kernel-inner	238.07 MB	238.07 MB	1
private-kernel-reset	260.32 MB	260.32 MB	1
private-kernel-tail	213.56 MB	213.56 MB	1
rollup-base-private	544.98 MB	544.98 MB	1
rollup-base-public	537.43 MB	537.43 MB	1
rollup-block-root	1440 MB	1440 MB	1
rollup-merge	366.5 MB	366.5 MB	1
rollup-root	372.43 MB	372.43 MB	1
semaphore_depth_10	91.5 MB	91.5 MB	1

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

[jfecher]

Pedantic: maybe we should use sub over minus since there's an existing SSA sub instruction? Or if we want to be more clear it's an offset we could also say , offset -3

[asterite]

maybe we should use sub over minus

I'll change it to sub 👍

[jfecher]

LGTM

[asterite]

Regarding minus vs sub: right now sub is used like v0 = sub v1, v2. With index Field 3 sub 1 it wouldn't read as clear as index Field 3 minus 1, so I'm not entirely convinced reusing sub is better, especially since SSA is usually produced by the compiler. We could use offset -3 but I think that's also less clear that x minus y. What do you think?

[asterite]

I'll merge this as-is for now, to try the ArraySet optimization. The SSA syntax is easy to change, so we could do that in a follow-up PR if wanted.

[asterite]

I started working on the ArraySet case but I'm not familiar with the brillig part of this so at this point I think the optimization wasn't done there for a reason. @vezenovm Do you know?

[vezenovm]

I started working on the ArraySet case but I'm not familiar with the brillig part of this so at this point I think the optimization wasn't done there for a reason. @vezenovm Do you know?

I can't remember entirely, but I think I just was trying to be iterative with the work as at the time we had more logic inside of our Brillig gen for array operations (e.g. array index OOB validation) and array set is a more complicated operation than array get. Looking at Brillig gen again this is the code in convert_ssa_array_set where this same optimization can be applied:

        // Then set the value in the newly created array
        let items_pointer =
            self.brillig_context.codegen_make_array_or_vector_items_pointer(destination_for_store);

        self.brillig_context.codegen_store_with_offset(
            items_pointer,
            index_register,
            value_variable.extract_register(),
        );

I think it was just an oversight that I never also applied it to array set. But let me know if you run into any issues.

[asterite]

Thanks! Yes, yesterday I was about to implement it and noticed the "array set" part involves a lot more brillig code and I got confused. I'll give it a try!