-
Notifications
You must be signed in to change notification settings - Fork 122
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
[EC] Unify point addition for P-256/384/521 #1602
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1602 +/- ##
==========================================
- Coverage 78.10% 78.07% -0.03%
==========================================
Files 562 562
Lines 94654 94575 -79
Branches 13574 13570 -4
==========================================
- Hits 73927 73838 -89
- Misses 20133 20145 +12
+ Partials 594 592 -2 ☔ View full report in Codecov by Sentry. |
ctx->sub(y_out, y_out, s1j); | ||
ctx->sub(y_out, y_out, s1j); | ||
|
||
cmovznz(x_out, ctx->felem_num_limbs, z1nz, x2, x_out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be slightly more efficient to keep the original order as, for example, x_out would remain in registers/cache before sending it or x1 to x_3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it can make any difference in performance because: 1) the data is so small that everything is in cache anyway; 2) cmovznz
is a function call so if not inlined it can't reuse values in registers. I'd like to keep this order for clarity if you don't mind?
Issues:
CryptoAlg-2409
Description of changes:
Implement and use a single version of point addition
for implementations of NIST curves P-384, P-521, and
Fiat-crypto based implementation of P-256. The change
does not affect performance.
Call-outs:
I verified the performance was not affected on Graviton 3, Intel, and M1 CPUs. Example for M1:
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.