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

PROOF-OF-CONCEPT Replace x86_64 asm by CryptOpt output #1254

Conversation

real-or-random
Copy link
Contributor

@real-or-random real-or-random commented Mar 30, 2023

@dderjoel

It managed to convince autotools to compile your asm. It seems slower than our C code, at least on my machine:

12th Gen Intel(R) Core(TM) i7-1260P, TurboBoost disabled

./bench_internal field

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

this PR (e2684293b1b72a1ab5974a2864549cea2788cf95), --with-asm=x86_64 (which is the default)
field_sqr                     ,     0.0296    ,     0.0297    ,     0.0297 
field_mul                     ,     0.0339    ,     0.0341    ,     0.0344 

master (464a9115b4eda46b464d22829ece4f51985944bf), --with-asm=x86_64 (which is the default)
field_sqr                     ,     0.0270    ,     0.0271    ,     0.0272 
field_mul                     ,     0.0359    ,     0.0359    ,     0.0360 

master, --with-asm=no (464a9115b4eda46b464d22829ece4f51985944bf), gcc 12.2.1 -02:
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)
field_sqr                     ,     0.0236    ,     0.0238    ,     0.0240 
field_mul                     ,     0.0283    ,     0.0284    ,     0.0286

By the way, we should really get rid of our current ASM in the short term, at least for the field. GCC beats it significantly.

Co-authored-by: Tim Ruffing <crypto@timruffing.de>
@dderjoel
Copy link

  1. What are the magic incantations to convince Autotools?
  2. I've added the fiat-dettman-c code the benchmark is attached (but probably not too representative, as I cant compare against the asm, as I don't have the magic words for auto tools)

@dderjoel
Copy link

original C

$ ./bench_ecmult
Benchmark , Min(us) , Avg(us) , Max(us)

ecmult_gen , 18.4 , 18.6 , 18.8
ecmult_const , 36.3 , 36.7 , 39.0
ecmult_1p , 28.9 , 29.3 , 29.8
ecmult_0p_g , 20.2 , 20.6 , 23.7
ecmult_1p_g , 16.9 , 17.0 , 17.2
ecmult_multi_0p_g , 20.0 , 20.3 , 20.7

@dderjoel
Copy link

cryptopt asm

$ ./bench_ecmult
Benchmark , Min(us) , Avg(us) , Max(us)

ecmult_gen , 17.7 , 18.1 , 19.2
ecmult_const , 34.1 , 35.1 , 37.9
ecmult_1p , 27.2 , 28.1 , 29.4
ecmult_0p_g , 18.7 , 19.5 , 21.4
ecmult_1p_g , 16.2 , 16.9 , 18.6
ecmult_multi_0p_g , 18.8 , 19.4 , 20.2

@dderjoel
Copy link

dderjoel commented Mar 31, 2023

fiat-dettman c
./bench_ecmult
Benchmark , Min(us) , Avg(us) , Max(us)

ecmult_gen , 19.5 , 20.1 , 21.5
ecmult_const , 38.9 , 40.4 , 41.4
ecmult_1p , 30.3 , 31.2 , 33.5
ecmult_0p_g , 20.8 , 21.3 , 24.0
ecmult_1p_g , 17.5 , 18.1 , 18.7
ecmult_multi_0p_g , 22.1 , 23.2 , 25.3

@real-or-random
Copy link
Contributor Author

Tracked in #1261.

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.

2 participants