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

Optimize Skipping of 0-bits In mulmuladd #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Dec 15, 2023

When investigating the mulmuladd issue, I noticed that at the start of the function, we have a loop to skip index for leading 0s in both u and v.

Currently, it was computing the a value 0bXY (where X bit being set indicates that v{index} is set, and Y bit indicates that u{index} is set) and storing it to a temporary variable (T4) for checking if the leading bit was 0 or not. This PR changes the logic to store it directly to the zz variable instead of having it be recomputed after the loop finishes.

At first, this was implemented by changing the condition of the loop to "is either v{index} or u{index} set?" but computing zz directly should be slightly better on average (if we assume even bit distribution, there is only a 25% chance this will save gas).

Also, eq(X, 0) -> iszero(X) for slightly smaller code and less gas used.

What is the best way to benchmark the difference?

@mmv08
Copy link
Contributor

mmv08 commented Dec 15, 2023

What is the best way to benchmark the difference?

Not sure about the project's benchmarking conventions, but perhaps you can run the tests with the --gas-report flag. More on it here: https://book.getfoundry.sh/forge/gas-reports

@rdubois-crypto
Copy link
Owner

Could you ensure linting and rebase with previous PR to ensure CI's OK?

For the rest OK for me.

@nlordell
Copy link
Contributor Author

@rdubois-crypto - should be rebased to latest master.

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.

3 participants