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: add (x | 1) optimization for booleans #6795

Merged
merged 8 commits into from
Dec 12, 2024
Merged

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 12, 2024

Description

Problem*

Resolves an issue in a slack thread by @michaeljklein

Summary*

Breakout of a change in #6771 where slice pop back seemed to be using all of its ctrl_typevars for a tuple instead of the one element that was actually being popped. This only affects the ArrayGet though which is subsequently optimized out.

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.

@jfecher jfecher requested a review from a team December 12, 2024 17:38
Copy link
Contributor

github-actions bot commented Dec 12, 2024

Changes to number of Brillig opcodes executed

Generated at commit: 9f0fab59e72cb848619959527679ebf75db780c1, compared to commit: 919149d3413be5232b33611094687fdb5fd86086

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bool_or -2 ✅ -8.00%

Full diff report 👇
Program Brillig opcodes (+/-) %
bool_or 23 (-2) -8.00%

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Changes to Brillig bytecode sizes

Generated at commit: 9f0fab59e72cb848619959527679ebf75db780c1, compared to commit: 919149d3413be5232b33611094687fdb5fd86086

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bool_or -4 ✅ -12.90%

Full diff report 👇
Program Brillig opcodes (+/-) %
bool_or 27 (-4) -12.90%

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Peak Memory Sample

Program Peak Memory
keccak256 79.14M
workspace 121.92M
regression_4709 295.98M
ram_blowup_regression 2.44G
private-kernel-tail 210.43M
private-kernel-reset 862.21M
private-kernel-inner 307.94M
parity-root 175.78M

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.528s 5%
regression_4709 0m0.832s 9%
ram_blowup_regression 0m17.393s 1%
private-kernel-tail 0m1.154s -7%
private-kernel-reset 0m9.373s 3%
private-kernel-inner 0m2.803s 3%
parity-root 0m0.911s -4%
noir-contracts 2m58.779s 9%

Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

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

LGTM

@jfecher
Copy link
Contributor Author

jfecher commented Dec 12, 2024

$ cargo t partial_constant_folding
   Compiling noirc_evaluator v1.0.0-beta.0 (/home/jake/Code/Noir/noir/compiler/noirc_evaluator)
    Finished test [optimized + debuginfo] target(s) in 1.09s
     Running unittests src/lib.rs (/home/jake/Code/Noir/noir/target/debug/deps/noirc_evaluator-4693fae089867dbc)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 189 filtered out; finished in 0.00s

I must be missing something obvious. Not sure why partial_constant_folding is failing in CI but not being run at all locally for me.

Edit: Maybe it is the #[cfg(feature = "bn254")]

@jfecher jfecher changed the title fix: Fix possible type error & add (x | 1) optimization for booleans feat: add (x | 1) optimization for booleans Dec 12, 2024
@jfecher jfecher enabled auto-merge December 12, 2024 19:45
@jfecher jfecher added this pull request to the merge queue Dec 12, 2024
Merged via the queue into master with commit feec2a1 Dec 12, 2024
64 checks passed
@jfecher jfecher deleted the jf/fix-simplify-types branch December 12, 2024 21:22
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)
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