Skip to content

Scalar 4x64 performance improvements #453

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

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

Conversation

peterdettman
Copy link
Contributor

(Not for immediate merge)

In #452 I noted that sqr and mul take about the same time in my config (OSX, 64-bit, no-asm, -O3 -march=native), so this is a quick attempt to speed up _scalar_sqr. This initial commit rewrites _scalar_sqr_512 for an ~ 8% improvement in _scalar_sqr. Second opinions/measurements would be appreciated.

It seems from the measurements that _scalar_reduce_512 is the real heavyweight here, so I'll be trying to re-implement that next.

I can rewrite in terms of macros (the current local code style) prior to any merge.

@dcousens
Copy link
Contributor

@peterdettman other than inlining, why is this faster?

@peterdettman
Copy link
Contributor Author

@dcousens The existing code is using macros not functions, so I doubt there is any benefit from writing inline code per se. Pre-loading a->d[0] etc. I don't think makes much difference (presumably the compiler does it anyway). Did you mean something else?

My assumption would be that the improvement is coming from using several uint128_t accumulators instead of the existing "160 bit accumulator" - muladd2 is quite awkward as a result, and presumably where the extra time is going.

@peterdettman
Copy link
Contributor Author

New commit rewrites secp256k1_reduce_512. Cumulative speed improvement for _scalar_inverse is now measured at >30%, bench_sign measurements improved by ~7-8%.

The "trick" used with p4 (see lines 588, 611 and comments) warrants careful review.

@sipa
Copy link
Contributor

sipa commented Apr 25, 2017

Benchmarked bench_verify with GMP and asm disabled on a i7-6820HQ CPU, pegged to 2.6GHz:

  • Master: 101μs
  • This PR: 98μs

@ofek
Copy link

ofek commented May 22, 2017

Is this ready to be merged?

@peterdettman
Copy link
Contributor Author

No, it needs thorough review of the carry handling and modular reduction, which can have very subtle bugs that random tests won't catch. I'd also like to get around to rewriting _scalar_mul in the same spirit, although I expect less improvement from that, and probably putting the code back into a cleaner macro-style like the original code.

@sipa
Copy link
Contributor

sipa commented May 8, 2023

@peterdettman Are you still interested in this? It seems like a reasonable improvement, and I'm willing to look into verifying that the double-correction trick always yields the right answer. I do think we'll want it in macro-style though.

@real-or-random
Copy link
Contributor

I do think we'll want it in macro-style though.

Or instead of macros, actual (inlinable) C functions with type checking and all that modern stuff. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants