Refactor: Use SIMD4x64 in the x86 SHA-512 compression function - #5779
Merged
randombit merged 1 commit intoAug 1, 2026
Merged
Conversation
…nction Resolves the TODO in sha2_64_x86.cpp by replacing the generic AVX2 intrinsics (loads, stores, additions, permute4x64) with the SIMD_4x64 wrapper. The SHA-512 ISA instructions (sha512rnds2/msg1/msg2) have no wrapper equivalent and remain as raw intrinsics. No functional change intended; verified against the known answer tests for SHA-384, SHA-512 and SHA-512/256 under Intel SDE.
There was a problem hiding this comment.
Pull request overview
Refactors the x86 SHA-512 compression implementation to use Botan’s existing SIMD_4x64 wrapper instead of direct AVX2 intrinsics where possible, aiming to keep behavior unchanged while reducing raw-intrinsics surface area.
Changes:
- Replaces explicit byte-swap/shuffle message loads with
SIMD_4x64::load_be, aligning with the SHA-512 big-endian message word convention. - Replaces state/constant loads and arithmetic with
SIMD_4x64operations (load_le,store_le,+=,+) while keeping SHA-512 ISA intrinsics as raw operations. - Introduces file-local helpers (
sha512_msg1,sha512_msg2) to encapsulate.raw()/extracti128plumbing for the vendor-specific SHA-512 message schedule instructions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
You're welcome, and thank you for the quick response. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves the "Use SIMD_4x64 here" TODO message in the
sha2_64_x86.cpp. It's a refactoring and doesn't aim for a functional change: general AVX2 intrinsic functions are replaced with the existing SIMD_4x64 wrapper, while anything without a wrapper equivalent remains as raw intrinsic functions.The structure of the change is as follows:
Message loads now use
load_be, matching the big-endian word convention of FIPS 180-4 and replacing the previous explicitbswap_mask + shuffle_epi8combination. Round constants and the digest state are native uint64_t words and therefore useload_le/store_le, matching the unaligned loads of the previous code.The SHA-512 ISA instructions (sha512rnds2, sha512msg1, sha512msg2) are vendor-specific and have no portable wrapper equivalent, so they remain raw. The two message-schedule instructions are wrapped in file-local helpers so that the
.raw()plumbing and the extracti128 detail (per the Intel Intrinsics Guide, msg1 only consumes the low two words of its second operand) appear exactly once.Besides the SHA instructions, the cross-lane shuffles in permute_state (shuffle_epi32, permute2x128) and the epi32-granularity blend in the message expansion have no SIMD_4x64 counterpart. Adding such methods to the shared header would touch its ISA attributes and widen the scope of this patch, so I left them out. The NOLINT(portability-simd-intrinsics) block is retained and now covers only these helpers.
Validation
Since my processor (Alder Lake) doesn't support the SHA-512 ISA extension, I tested using Intel SDE. Among the CPU flags defined under SDE is intel_sha512, and the test counts for SHA-384 (168 → 336), SHA-512 (3554 → 7108), and SHA-512/256 (46 → 92) were exactly doubled compared to a local run. Therefore, I believe it is correct:
sde64 -future -- ./botan-test --verbose hashI would like to help as soon as possible with any additional tests and adjustments needed during the review. I am also open to additional resources and code examples for further information.
Best regards.