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

DRAFT: Replace Field Arithmetic #1260

Closed
wants to merge 4 commits into from
Closed

Conversation

dderjoel
Copy link

@dderjoel dderjoel commented Apr 5, 2023

So in this PR, the C implementation is replaced by the Fiat Cryptography C implementation and the x86-46 implementation is replaced with CryptOpt optimized versions.

I'll report the performance results soon.

  1. (this PR) ./configure (default) should use the optimized x86-asm
  2. (this PR) ./configure --with-x86=no should use the Fiat-C version
  3. (2bca0a5) ./configure
  4. (2bca0a5) ./configure --with-x86=no

Is that all we need? I then run ./bench_internal, reporting the field_{mul,sqr} rows as well as ./bench_ecmult reporting the ecmult_{gen,const,1p,0p_g} rows.

dderjoel and others added 4 commits March 31, 2023 00:21
--with-asm=no on Intel i5-8265U (8) @ 3.900GHz

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

scalar_add                    ,     0.00883   ,     0.00937   ,     0.0132
scalar_negate                 ,     0.00338   ,     0.00341   ,     0.00357
scalar_mul                    ,     0.0373    ,     0.0376    ,     0.0390
scalar_split                  ,     0.169     ,     0.171     ,     0.183
scalar_inverse                ,     1.74      ,     1.75      ,     1.76
scalar_inverse_var            ,     1.24      ,     1.24      ,     1.26
field_half                    ,     0.00281   ,     0.00284   ,     0.00294
field_normalize               ,     0.00965   ,     0.00977   ,     0.0101
field_normalize_weak          ,     0.00374   ,     0.00377   ,     0.00391
field_sqr                     ,     0.0147    ,     0.0150    ,     0.0161
field_mul                     ,     0.0192    ,     0.0195    ,     0.0209
field_inverse                 ,     1.73      ,     1.73      ,     1.75
field_inverse_var             ,     1.22      ,     1.24      ,     1.26
field_is_square_var           ,     1.57      ,     1.58      ,     1.60
field_sqrt                    ,     4.05      ,     4.10      ,     4.20
group_double_var              ,     0.128     ,     0.129     ,     0.132
group_add_var                 ,     0.307     ,     0.308     ,     0.312
group_add_affine              ,     0.249     ,     0.252     ,     0.268
group_add_affine_var          ,     0.223     ,     0.223     ,     0.227
group_add_zinv_var            ,     0.244     ,     0.247     ,     0.254
group_to_affine_var           ,     1.33      ,     1.33      ,     1.35
wnaf_const                    ,     0.241     ,     0.246     ,     0.262
ecmult_wnaf                   ,     0.538     ,     0.543     ,     0.554
hash_sha256                   ,     0.275     ,     0.278     ,     0.292
hash_hmac_sha256              ,     1.08      ,     1.09      ,     1.13
hash_rfc6979_hmac_sha256      ,     5.96      ,     5.99      ,     6.17
context_create                ,     0.576     ,     0.582     ,     0.601
@sipa
Copy link
Contributor

sipa commented Apr 6, 2023

FWIW I opened this issue mit-plv/fiat-crypto#1582 about an optimization currently present in the libsecp256k1 field C code, which isn't in the fiat-crypto output (nor in the libsecp256k1 current asm code).

@real-or-random
Copy link
Contributor

Let me close this here for now. I opened #1261 to track integration of fiat-crypto + CryptOpt. :)

@real-or-random real-or-random marked this pull request as draft April 6, 2023 05:39
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