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

chore: implement is_zero in terms of AcirVars #3655

Closed
wants to merge 11 commits into from

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Dec 1, 2023

Description

Problem*

Resolves

Summary*

Throwing up an old PR I have, just want to check what CI makes of it.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] 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.

@TomAFrench TomAFrench closed this Dec 1, 2023
@TomAFrench TomAFrench reopened this Dec 1, 2023
Copy link
Contributor

github-actions bot commented Dec 1, 2023

Changes to circuit sizes

Generated at commit: b2ce36cfd4f40d26dccf99a1cbc8d4f6c734bfd5, compared to commit: bc9a44f285e0569825a307b06ee8acd93461c87e

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
conditional_1 +1 ❌ +0.01% 0 ➖ 0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
conditional_1 6,917 (+1) +0.01% 38,799 (0) 0.00%

@jfecher
Copy link
Contributor

jfecher commented Dec 8, 2023

What is the status of this PR?

@TomAFrench TomAFrench marked this pull request as ready for review December 10, 2023 23:23
@TomAFrench
Copy link
Member Author

TomAFrench commented Dec 10, 2023

I think it's ready for review. I'm not dead set on this going in but my feelings on acir_variable.rs vs generated_acir.rs is that we should encode the logic of various operations in acir_variable.rs as much as possible so that we can make use of the optimisations which are present there. (Granted we're not benefiting from it here)

In comparison generated_acir.rs is relatively "dumb" as it needs actual witnesses, etc. so we should avoid it where possible and just have it for handling black box function calls, etc.

AztecBot pushed a commit that referenced this pull request Jan 2, 2024
sirasistant pushed a commit that referenced this pull request Jan 4, 2024
@TomAFrench TomAFrench closed this Jan 14, 2024
@TomAFrench TomAFrench deleted the tf/is-zero-acir-var branch January 14, 2024 22:06
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