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

Add the dettman square operation #1576

Merged
merged 3 commits into from
Mar 19, 2023
Merged

Add the dettman square operation #1576

merged 3 commits into from
Mar 19, 2023

Conversation

OwenConoly
Copy link
Collaborator

Turns out that the squaring algorithm is essentially identical to the multiplication algorithm. To transform the mul function into the square function, I just had to replace Associational.mul with Associational.square.

@OwenConoly
Copy link
Collaborator Author

I benchmarked the generated C implementation for square using the tools in https://github.com/bitcoin-core/secp256k1/. It's significantly faster than the mul operation, which is reassuring. I wasn't able to discern a significant difference between the performance of the generated C implementation vs. that of the handwritten assembly version.

Copy link
Collaborator

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

Looks good! Here are a couple suggestions that I think will make the organization slightly cleaner

src/Arithmetic/DettmanMultiplication.v Outdated Show resolved Hide resolved
src/PushButtonSynthesis/DettmanMultiplication.v Outdated Show resolved Hide resolved
src/PushButtonSynthesis/DettmanMultiplication.v Outdated Show resolved Hide resolved
@JasonGross
Copy link
Collaborator

What's up with the

fatal: detected dubious ownership in repository at '/github/workspace/rewriter'
To add an exception for this directory, call:

	git config --global --add safe.directory /github/workspace/rewriter

errors? (Does the branch just need to be rebased, or is something bigger going on?)

@OwenConoly
Copy link
Collaborator Author

I don't know what could've caused that.

@JasonGross
Copy link
Collaborator

@JasonGross
Copy link
Collaborator

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