Skip to content

Commit 509bb59

Browse files
davidbenBoringssl LUCI CQ
authored andcommitted
Revert "Speed up sha512 on x86" and update comments
This reverts commit fcef13a and updates the discussion based on current understanding of AMD Zen CPUs. See also https://boringssl-review.googlesource.com/c/boringssl/+/73567/comments/bfb27b7f_b05338a1 Bug: 42290564 Change-Id: If743ce2a16592e4b56dc813c8fc13e9dc1a40b70 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76227 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
1 parent b8291f8 commit 509bb59

File tree

1 file changed

+15
-11
lines changed

1 file changed

+15
-11
lines changed

crypto/fipsmodule/sha/internal.h

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,9 @@ void sha1_block_data_order_ssse3(uint32_t state[5], const uint8_t *data,
8383

8484
#define SHA1_ASM_AVX
8585
inline int sha1_avx_capable(void) {
86-
// Pre-Zen AMD CPUs had slow SHLD/SHRD; Zen added the SHA extension; see the
87-
// discussion in sha1-586.pl.
86+
// AMD CPUs have slow SHLD/SHRD. See also the discussion in sha1-586.pl.
8887
//
89-
// TODO(davidben): Should we enable SHAEXT on 32-bit x86?
88+
// TODO(crbug.com/42290564): Should we enable SHAEXT on 32-bit x86?
9089
// TODO(davidben): Do we need to check the FXSR bit? The Intel manual does not
9190
// say to.
9291
return CRYPTO_is_AVX_capable() && CRYPTO_is_intel_cpu() &&
@@ -106,10 +105,9 @@ void sha256_block_data_order_ssse3(uint32_t state[8], const uint8_t *data,
106105

107106
#define SHA256_ASM_AVX
108107
inline int sha256_avx_capable(void) {
109-
// Pre-Zen AMD CPUs had slow SHLD/SHRD; Zen added the SHA extension; see the
110-
// discussion in sha1-586.pl.
108+
// AMD CPUs have slow SHLD/SHRD. See also the discussion in sha1-586.pl.
111109
//
112-
// TODO(davidben): Should we enable SHAEXT on 32-bit x86?
110+
// TODO(crbug.com/42290564): Should we enable SHAEXT on 32-bit x86?
113111
// TODO(davidben): Do we need to check the FXSR bit? The Intel manual does not
114112
// say to.
115113
return CRYPTO_is_AVX_capable() && CRYPTO_is_intel_cpu() &&
@@ -148,8 +146,8 @@ void sha1_block_data_order_avx2(uint32_t state[5], const uint8_t *data,
148146

149147
#define SHA1_ASM_AVX
150148
inline int sha1_avx_capable(void) {
151-
// Pre-Zen AMD CPUs had slow SHLD/SHRD; Zen added the SHA extension; see the
152-
// discussion in sha1-586.pl.
149+
// AMD CPUs have slow SHLD/SHRD. See also the discussion in sha1-586.pl. Zen
150+
// added the SHA extension, so this is moot on newer AMD CPUs.
153151
return CRYPTO_is_AVX_capable() && CRYPTO_is_intel_cpu();
154152
}
155153
void sha1_block_data_order_avx(uint32_t state[5], const uint8_t *data,
@@ -168,8 +166,8 @@ inline int sha256_hw_capable(void) {
168166

169167
#define SHA256_ASM_AVX
170168
inline int sha256_avx_capable(void) {
171-
// Pre-Zen AMD CPUs had slow SHLD/SHRD; Zen added the SHA extension; see the
172-
// discussion in sha1-586.pl.
169+
// AMD CPUs have slow SHLD/SHRD. See also the discussion in sha1-586.pl. Zen
170+
// added the SHA extension, so this is moot on newer AMD CPUs.
173171
return CRYPTO_is_AVX_capable() && CRYPTO_is_intel_cpu();
174172
}
175173
void sha256_block_data_order_avx(uint32_t state[8], const uint8_t *data,
@@ -181,7 +179,13 @@ void sha256_block_data_order_ssse3(uint32_t state[8], const uint8_t *data,
181179
size_t num);
182180

183181
#define SHA512_ASM_AVX
184-
inline int sha512_avx_capable(void) { return CRYPTO_is_AVX_capable(); }
182+
inline int sha512_avx_capable(void) {
183+
// AMD CPUs have slow SHLD/SHRD. See also the discussion in sha1-586.pl.
184+
//
185+
// TODO(crbug.com/42290564): Fixing and enabling the AVX2 implementation would
186+
// mitigate this on newer AMD CPUs.
187+
return CRYPTO_is_AVX_capable() && CRYPTO_is_intel_cpu();
188+
}
185189
void sha512_block_data_order_avx(uint64_t state[8], const uint8_t *data,
186190
size_t num);
187191

0 commit comments

Comments
 (0)