Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(perf): Follow array sets backwards in array set from get optimization #6208

Merged
merged 16 commits into from
Oct 4, 2024

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Oct 2, 2024

Description

Problem*

Part of general effort reduce Brillig bytecode sizes

Summary*

Follow-up to #6207

From comments inside the PR:

/// If we have an array set whose value is from an array get on the same array at the same index,
/// we can simplify that array set to the array we were looking to perform an array set upon.
///
/// Simple case:
/// v3 = array_get v1, index v2
/// v5 = array_set v1, index v2, value v3
///
/// If we could not immediately simplify the array set from its value, we can try to follow
/// the array set backwards in the case we have constant indices:
///
/// v3 = array_get v1, index 1
/// v5 = array_set v1, index 2, value [Field 100, Field 101, Field 102]
/// v7 = array_set mut v5, index 1, value v3
///
/// We want to optimize `v7` to `v5`. We see that `v3` comes from an array get to `v1`. We follow `v5` backwards and see an array set
/// to `v1` and see that the previous array set occurs to a different constant index.
///
/// For each array set:
/// - If the index is non-constant we fail the optimization since any index may be changed.
/// - If the index is constant and is our target index, we conservatively fail the optimization.
/// - Otherwise, we check the array value of the `array_set``. We will refer to this array as array'.
///   In the case above, array' is `v1` from `v5 = array set ...`
///   - If the original `array_set` value comes from an `array_get`, check the array in that `array_get` against array'. 
///   - If the two values are equal we can simplify.
///     - Continuing the example above, as we have `v3 = array_get v1, index 1`, `v1` is
///       what we want to check against array'. We now know we can simplify `v7` to `v5` as it is unchanged.
///   - If they are not equal, recur marking the current `array_set` array as the new array id to use in the checks

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.

@vezenovm vezenovm requested a review from a team October 2, 2024 19:45
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Changes to Brillig bytecode sizes

Generated at commit: 1132f9c0fcc700897a1508a3c780b34818885786, compared to commit: 24309200f600ad20a51d9f2c6c53849466fccda4

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
fold_complex_outputs -115 ✅ -17.67%

Full diff report 👇
Program Brillig opcodes (+/-) %
fold_complex_outputs 536 (-115) -17.67%

@vezenovm vezenovm requested review from TomAFrench and a team October 3, 2024 17:37
@vezenovm vezenovm added the run-external-checks Trigger CI job to run tests on external repos label Oct 3, 2024
@TomAFrench
Copy link
Member

Note that run-external-checks only runs nargo check on aztec packages so it'll only find issues which appear in the frontend. It won't pick up any SSA errors so you'll need to check that manually (or we can compile the contracts)

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

I think it would be good to have some more documentation within the function about our reasoning about what we're doing at particular steps.

@vezenovm vezenovm removed the run-external-checks Trigger CI job to run tests on external repos label Oct 4, 2024
vezenovm and others added 5 commits October 4, 2024 10:41
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@TomAFrench TomAFrench added this pull request to the merge queue Oct 4, 2024
Merged via the queue into master with commit 999071b Oct 4, 2024
47 checks passed
@TomAFrench TomAFrench deleted the mv/follow-array-set-in-array-set-from-get branch October 4, 2024 18:41
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 5, 2024
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 6, 2024
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 6, 2024
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
…lang/noir#5994)

fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
feat(test): Fuzz test stdlib hash functions (noir-lang/noir#6233)
fix: handle nested arrays in calldata (noir-lang/noir#6232)
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 7, 2024
feat(test): Fuzz test stdlib hash functions (noir-lang/noir#6233)
fix: handle nested arrays in calldata (noir-lang/noir#6232)
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 8, 2024
feat(test): Fuzz test stdlib hash functions (noir-lang/noir#6233)
fix: handle nested arrays in calldata (noir-lang/noir#6232)
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 8, 2024
feat(test): Fuzz test stdlib hash functions (noir-lang/noir#6233)
fix: handle nested arrays in calldata (noir-lang/noir#6232)
feat: add more `Type` and `UnresolvedType` methods (noir-lang/noir#5994)
fix: Panic on composite types within databus (noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature (noir-lang/noir#6226)
sirasistant added a commit to AztecProtocol/aztec-packages that referenced this pull request Oct 8, 2024
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: handwritten parser (noir-lang/noir#6180)
feat(test): Fuzz test stdlib hash functions
(noir-lang/noir#6233)
fix: handle nested arrays in calldata
(noir-lang/noir#6232)
feat: add more `Type` and `UnresolvedType` methods
(noir-lang/noir#5994)
fix: Panic on composite types within databus
(noir-lang/noir#6225)
feat(perf): Follow array sets backwards in array set from get
optimization (noir-lang/noir#6208)
fix: check for Schnorr null signature
(noir-lang/noir#6226)
END_COMMIT_OVERRIDE

---------

Co-authored-by: sirasistant <sirasistant@gmail.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