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: improve readability and document mul_with_witness #1268

Closed
wants to merge 6 commits into from
Closed

chore: improve readability and document mul_with_witness #1268

wants to merge 6 commits into from

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented May 2, 2023 •
edited
Loading

Related issue(s)

Resolves #

Description

Summary of changes

mul_with_witness currently creates an intermediate witness to store the value of an expression except in the following situations:

  • a is degree 2 and b is degree 0
  • a is degree 0 and b is degree 2

In general we want the sum of degrees of a and b to be 2 or less. We're currently missing the situation where a and b are both degree 1. I've then added a early return for this case so we can avoid extra witnesses in this case.

This PR adds documentation and improves readability of the mul_with_witness function.

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@TomAFrench TomAFrench changed the title feat: avoid creating extra intermediate variables in expression multiplication feat: Avoid creating extra intermediate variables in expression multiplication May 2, 2023
guipublic
guipublic requested changes May 2, 2023
View reviewed changes
Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

This PR is wrong as the previous code does not generate intermediate variables in multiplication of degree 1 expressions.

@TomAFrench TomAFrench changed the title feat: Avoid creating extra intermediate variables in expression multiplication chore: improve readability and document mul_with_witness May 2, 2023
@TomAFrench
Copy link
Member Author

Ah, you're right. I must have gotten tripped up by all the double negatives. I've removed the early return and focused the PR on readability/docs.

@TomAFrench TomAFrench requested a review from guipublic May 2, 2023 11:17
TomAFrench added 3 commits May 2, 2023 17:41
@TomAFrench
Merge branch 'master' into mul-with-witness-optimisation
36cd291 
* master:
  feat: add integration tests for bitshift operators (#1272)
  chore: Replace explicit if-elses with `FieldElement::from<bool>()` for boolean fields (#1266)
  chore(noir): constrain expr; -> assert(expr); (#1276)
  chore: fix clippy warning (#1270)
@TomAFrench
Merge branch 'master' into mul-with-witness-optimisation
805fa3a
@TomAFrench
chore: move comments across to new SSA
f4b73c7
@TomAFrench
Copy link
Member Author

Superceded by #1980

@TomAFrench TomAFrench closed this Jul 20, 2023
@TomAFrench TomAFrench deleted the mul-with-witness-optimisation branch November 20, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guipublic guipublic Awaiting requested review from guipublic

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

Successfully merging this pull request may close these issues.
2 participants
@TomAFrench @guipublic