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.

Sign up for GitHub

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

Jump to bottom

chore: add compile-time assertions on generic arguments of stdlib functions #6981

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

chore: add compile-time assertions on generic arguments of stdlib functions #6981

wants to merge 10 commits into from

Conversation

michaeljklein
Copy link
Contributor

Description

Problem*

Resolves #6962

Summary*

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.

@michaeljklein michaeljklein changed the title Add compile-time assertions on generic arguments of stdlib functions chore: add compile-time assertions on generic arguments of stdlib functions Jan 7, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025 •
edited
Loading

Compilation Report

Program Compilation Time %
sha256_regression 1.254s -3%
regression_4709 0.805s 0%
ram_blowup_regression 18.020s -1%
rollup-root 3.320s -7%
rollup-block-merge 3.354s -5%
rollup-base-public 41.860s -4%
rollup-base-private 13.100s -2%
private-kernel-tail 1.082s 2%
private-kernel-reset 6.932s -2%
private-kernel-inner 2.014s -6%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025 •
edited
Loading

Execution Report

Program Execution Time %
sha256_regression 0.054s 5%
regression_4709 0.001s 0%
ram_blowup_regression 0.628s -1%
rollup-root 0.105s 0%
rollup-block-merge 0.105s -1%
rollup-base-public 1.224s -1%
rollup-base-private 0.457s 0%
private-kernel-tail 0.019s -6%
private-kernel-reset 0.318s -1%
private-kernel-inner 0.071s 0%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025 •
edited
Loading

Compilation Memory Report

Program Peak Memory
keccak256 78.25M
workspace 123.67M
regression_4709 424.22M
ram_blowup_regression 1.58G
rollup-base-public 4.85G
rollup-base-private 1.26G
private-kernel-tail 207.25M
private-kernel-reset 669.32M
private-kernel-inner 294.58M

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025 •
edited
Loading

Execution Memory Report

Program Peak Memory
keccak256 74.81M
workspace 123.67M
regression_4709 316.13M
ram_blowup_regression 512.68M
rollup-base-public 480.70M
rollup-base-private 326.71M
private-kernel-tail 180.56M
private-kernel-reset 245.47M
private-kernel-inner 208.83M

TomAFrench
TomAFrench requested changes Jan 9, 2025
View reviewed changes
noir_stdlib/src/field/mod.nr
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't split into safe and unsafe variants but instead have a check that the decomposition is canonical similar to how we do it for byte decompositions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. How would a check that the decomposition is canonical work at compile-time? I would expect to check that it's canonical in Add feature-gated runtime assertions to stdlib functions #6964
  2. R.e. avoiding splitting into safe and unsafe versions, do you mean to move these checks into the builtin function implementations?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025 •
edited
Loading

Changes to Brillig bytecode sizes

Generated at commit: 096ddd17ffd8c5a8d55c83355a234e10bbed8242, compared to commit: db28cb9ffb710c286b54dbfcf57292ae3dffb03d

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bigint +3 ❌ +0.14%
Full diff report 👇
Program Brillig opcodes (+/-) %
bigint 2,105 (+3) +0.14%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomAFrench TomAFrench TomAFrench requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add compile-time assertions on generic arguments of stdlib functions
2 participants
@michaeljklein @TomAFrench