Move dynamic dispatching to C for x86_64-mont5.pl #2592
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2592 +/- ##
=======================================
Coverage 78.82% 78.83%
=======================================
Files 667 668 +1
Lines 114077 114118 +41
Branches 16047 16052 +5
=======================================
+ Hits 89924 89962 +38
- Misses 23379 23381 +2
- Partials 774 775 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
| bool is_assembler_too_old = false; | ||
| bool is_assembler_too_old_avx512 = false; | ||
| bool ifma_avx512 = false; | ||
| bool is_bn_mulx4x_mont_gather5 = false; |
There was a problem hiding this comment.
I'm not sure that we need these five values defined here, we only define capabilities here?
There was a problem hiding this comment.
This matches how we do it for the other dynamic dispatching tests. The only difference is they're not value dependent like these are.
There was a problem hiding this comment.
not sure I understand. All other variables here refer to specific CPU capabilities like AES-HW, SHA-EXT, AVX, IFMA, etc., while bn_mulx4x_mont_gather5 is not a capability. Could you please explain why you think you need these variables defined instead of specifica CPU capabilities that are required for those functions to run?
There was a problem hiding this comment.
These features are CPU capability dependent but also value dependent. We would do the CPU capabilities as global booleans like the others and leave the value as a dynamic booleans inside the test.
|
|
||
| bn_mul_mont(r.data(), a.data(), b.data(), mont->N.d, mont->n0, words); | ||
|
|
||
| is_bn_mulx4x_mont_gather5 = bn_mulx4x_mont_gather5_capable(words); |
There was a problem hiding this comment.
if you use the same function bn_mulx4x_mont_gather5_capable for the test and the actual implementation then this test doesn't make much sense, it will always pass. The idea of dispatch tests is to determine if a specific combination of CPU capabilities (like ADX, BMI, BMI2, etc.) triggers the expected code path.
There was a problem hiding this comment.
We can pull it out but we'll also need to pull out the tests on the numeric value as well.
There was a problem hiding this comment.
I didn't mean to remove the test but to modify it such that it actually tests the dispatching logic. Take a look at this test for example: https://github.com/m271828/aws-lc/blob/64fcb96bb5764a02e4e2d79ff17cf202b8a53a18/crypto/impl_dispatch_test.cc#L486
There was a problem hiding this comment.
The dispatching function isn't externally reachable. We'd need to either duplicate the logic, export the dispatching function or call from a higher level (figuring out the right values to use).
| bool is_assembler_too_old = false; | ||
| bool is_assembler_too_old_avx512 = false; | ||
| bool ifma_avx512 = false; | ||
| bool is_bn_mulx4x_mont_gather5 = false; |
There was a problem hiding this comment.
not sure I understand. All other variables here refer to specific CPU capabilities like AES-HW, SHA-EXT, AVX, IFMA, etc., while bn_mulx4x_mont_gather5 is not a capability. Could you please explain why you think you need these variables defined instead of specifica CPU capabilities that are required for those functions to run?
|
|
||
| bn_mul_mont(r.data(), a.data(), b.data(), mont->N.d, mont->n0, words); | ||
|
|
||
| is_bn_mulx4x_mont_gather5 = bn_mulx4x_mont_gather5_capable(words); |
There was a problem hiding this comment.
I didn't mean to remove the test but to modify it such that it actually tests the dispatching logic. Take a look at this test for example: https://github.com/m271828/aws-lc/blob/64fcb96bb5764a02e4e2d79ff17cf202b8a53a18/crypto/impl_dispatch_test.cc#L486
| #include "crypto.h" | ||
|
|
||
| __attribute__((unused)) | ||
| void log_dispatch(size_t id); |
There was a problem hiding this comment.
this should not be in a public header. maybe it can go in crypto/internal.h?
4 participants
Issues:
Resolves P280855694
Addresses CryptoAlg-2964
Description of changes:
Move dynamic dispatching from assembly to C.
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.