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

fix: optimizer to keep track of changing opcode locations #6781

Merged
merged 10 commits into from
Dec 12, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 12, 2024

Description

Problem*

Resolves the issue found trying to merge AztecProtocol/aztec-packages#10483
Mirroring the commits added to that PR.

Summary*

Unlike transform_internal, optimize_internal did not take an acir_opcode_positions which would have allowed it to be chained. This caused the second iteration of the loop to reset acir_opcode_positions to the default values, which indicated no change when the AcirTransformationMap was derived from it. The consequence was that the assert_messages and debug information did not align with the new opcode locations, which changed in the first iteration.

Also changed test_cmd to apply the nargo::ops::transform optimisations, so we get to see any problems introduced there come up in tests. This actually brought out this regression in vecdequeue test, where an unexpected assertion message was returned.

Added an integration test to provoke the error fixed by the PR. In order for the ACIR not to eliminate all operations due to them working with constants known in the test code, I am calling it through the fuzzer, which makes the test look silly. To make it work required an update to the fuzz tester to anticipate tests that expect failures.

Additional Context

The way to debug this on aztec-packages went like this.
First build everything with ./bootstrap.sh, then in one terminal start an oracle:

export LOG_LEVEL=debug
cd aztec-packages/yarn-project/txe
yarn start

In another terminal run a test (one of the failing ones):

export NARGO=$(pwd)/aztec-packages/noir/noir-repo/target/release/nargo 
cd aztec-packages/noir-projects/noir-contracts
$NARGO test --silence-warnings --oracle-resolver http://localhost:8080 --package nft_contract transfer_to_public_failure_not_an_owner 

Then change something in the Noir codebase and recompile:

cd aztec-packages/noir 
./bootstrap.sh

With const MAX_OPTIMIZER_PASSES: usize = 1; the test passed, otherwise failed. The LOG_LEVEL=debug showed the index of the failing opcode. Curiously it was the same in both tests (the oracle is expected to fail, the test verifies it fails with the right message).

One of the things I tried was to print the ACIR opcodes with, with a change in the code so it prints the optimised version, after transformations:

$NARGO compile --package nft_contract --print-acir 

Comparing the ACIR indicated that 1 or 3 passes doesn't make any difference. Since it was also the same opcode, I started to suspect that it is actually the assertion message that gets lost.

With this fix the contract tests pass.

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.

@aakoshh aakoshh changed the title Fix optimizer to keep track of changing opcode locations fix: optimizer to keep track of changing opcode locations Dec 12, 2024
Copy link
Contributor

github-actions bot commented Dec 12, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.64M
workspace 121.88M
regression_4709 294.72M
ram_blowup_regression 2.44G
private-kernel-tail 208.79M
private-kernel-reset 862.88M
private-kernel-inner 307.69M
parity-root 174.43M

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.453s -9%
regression_4709 0m1.480s -4%
ram_blowup_regression 0m16.897s -2%
private-kernel-tail 0m1.508s 1%
private-kernel-reset 0m9.642s 5%
private-kernel-inner 0m2.788s 17%
parity-root 0m1.117s -5%
noir-contracts 2m51.576s 8%

@aakoshh aakoshh marked this pull request as ready for review December 12, 2024 02:00
@aakoshh aakoshh requested a review from TomAFrench December 12, 2024 02:03
@aakoshh aakoshh requested a review from guipublic December 12, 2024 09:56
@aakoshh aakoshh requested a review from asterite December 12, 2024 14:12
Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good!

@aakoshh aakoshh added this pull request to the merge queue Dec 12, 2024
Merged via the queue into master with commit 13c41d2 Dec 12, 2024
66 checks passed
@aakoshh aakoshh deleted the fix-optimizer-opcode-locations branch December 12, 2024 15:20
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 13, 2024
…iant code motion (noir-lang/noir#6782)

feat: add `(x | 1)` optimization for booleans (noir-lang/noir#6795)
feat: `nargo test -q` (or `nargo test --format terse`) (noir-lang/noir#6776)
fix: disable failure persistance in nargo test fuzzing (noir-lang/noir#6777)
feat(cli): Verify `return` against ABI and `Prover.toml` (noir-lang/noir#6765)
chore(ssa): Activate loop invariant code motion on ACIR functions (noir-lang/noir#6785)
fix: use extension in docs link so it also works on GitHub (noir-lang/noir#6787)
fix: optimizer to keep track of changing opcode locations (noir-lang/noir#6781)
fix: Minimal change to avoid reverting entire PR #6685 (noir-lang/noir#6778)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 13, 2024
…tion (noir-lang/noir#6782)

feat: add `(x | 1)` optimization for booleans (noir-lang/noir#6795)
feat: `nargo test -q` (or `nargo test --format terse`) (noir-lang/noir#6776)
fix: disable failure persistance in nargo test fuzzing (noir-lang/noir#6777)
feat(cli): Verify `return` against ABI and `Prover.toml` (noir-lang/noir#6765)
chore(ssa): Activate loop invariant code motion on ACIR functions (noir-lang/noir#6785)
fix: use extension in docs link so it also works on GitHub (noir-lang/noir#6787)
fix: optimizer to keep track of changing opcode locations (noir-lang/noir#6781)
fix: Minimal change to avoid reverting entire PR #6685 (noir-lang/noir#6778)
TomAFrench added a commit that referenced this pull request Dec 14, 2024
* master: (313 commits)
  chore: Do not print entire functions when running debug trace (#6814)
  chore(ci): Active rollup circuits in compilation report (#6813)
  feat(ssa): Bring back tracking of RC instructions during DIE (#6783)
  feat: add `nargo test --format json` (#6796)
  chore: Change Id to use a u32 (#6807)
  feat(ssa): Hoist MakeArray instructions during loop invariant code motion  (#6782)
  feat: add `(x | 1)` optimization for booleans (#6795)
  feat: `nargo test -q` (or `nargo test --format terse`) (#6776)
  fix: disable failure persistance in nargo test fuzzing (#6777)
  feat(cli): Verify `return` against ABI and `Prover.toml` (#6765)
  chore(ssa): Activate loop invariant code motion on ACIR functions (#6785)
  fix: use extension in docs link so it also works on GitHub (#6787)
  fix: optimizer to keep track of changing opcode locations (#6781)
  fix: Minimal change to avoid reverting entire PR #6685 (#6778)
  feat: several `nargo test` improvements (#6728)
  chore: Try replace callstack with a linked list (#6747)
  chore: Use `NumericType` not `Type` for casts and numeric constants (#6769)
  chore(ci): Extend compiler memory report to external repos (#6768)
  chore(ci): Handle external libraries in compilation timing report (#6750)
  feat(ssa): Implement missing brillig constraints SSA check (#6658)
  ...
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