Optimize crc32 & crc32c on NVIDIA Grace #2204
Conversation
krenzland
changed the title
|
@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
folly/hash/Checksum.cpp
Outdated
| @@ -77,6 +77,10 @@ uint32_t crc32_hw( | |||
| } | |||
|
|
|||
| bool crc32c_hw_supported() { | |||
| return crc32_hw_supported_sse42(); | |||
There was a problem hiding this comment.
This has a typo, missing the c after crc32.
Correct name should be crc32c_hw_supported_sse42
| @@ -105,7 +111,35 @@ static uint32_t gf_multiply_crc32_hw(uint64_t crc1, uint64_t crc2, uint32_t) { | |||
| return _mm_cvtsi128_si32(_mm_srli_si128(_mm_xor_si128(res3, res1), 4)); | |||
| } | |||
|
|
|||
| #else | |||
| #elif FOLLY_NEON && FOLLY_ARM_FEATURE_CRC32 && FOLLY_ARM_FEATURE_AES && FOLLY_ARM_FEATURE_SHA2 | |||
There was a problem hiding this comment.
This line is too long.
Can you resubmit after a clang-format?
Sorry, we should have this automated as part of a git hook or something, not sure why we don't have it.
There was a problem hiding this comment.
Okay. Can I suggest to add clang-format to https://github.com/facebook/folly/blob/main/CONTRIBUTING.md?
- Fix typo - Clang format all changed files
|
Thanks for the review! I forgot to add that this should be compiled with the flags |
|
@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@krenzland Hey after internal discussions, we would like to request to move your contributions under AFAIU, CMake should automatically pick them up, as we have auto_source with recurse. Thanks in advanced |
folly/hash/Checksum.cpp
Outdated
| #else | ||
| #elif FOLLY_ARM_FEATURE_CRC32 | ||
| uint32_t crc32_hw(const uint8_t* buf, size_t len, uint32_t crc) { | ||
| auto* buf_64 = reinterpret_cast<const uint64_t*>(buf); |
There was a problem hiding this comment.
This looks like unaligned access.
Note that while an architecture may support unaligned access, the language generally deems unaligned access of this form to be undefined behavior. The kind of undefined behavior that may end up being subject to miscompilations.
To avoid this unaligned access, do we need to start off with an optional round of each of __crc32b, __crc32h, __crc32w, and __crc32d? Alternatively, perhaps we can memcpy from buf instead of reinterpret_cast - modern compilers recognize this idiom and lower to mov or ldr instructions without emitting calls to memcpy.
Here and crc32c_hw below.
There was a problem hiding this comment.
Thanks, you're totally right. Unaligned access isn't directly forbidden in C++ but my implementation is incorrect by the strict aliasing rule. While this is typically fine when operating on bytes my code is still incorrect.
The memcpy version is a good idea. It's obviously correct and at a first glance seems to lead to (nearly) the same code for GCC/clang. I'll test it and update the PR.
krenzland
force-pushed
the
optimize-crc32c
branch
from
81c22f5
to
4f99658
Compare
|
@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request adds hardware accelerated routines for CRC32 and CRC32C for Arm AARCH64 CPUs. The changes here have been tested on NVIDIA Grace.
In detail, it contains routines for:
gf_multiply_crc32c_hwandgf_multiply_crc32_hwfunctions used in routines to merge partial CRC checksums. These functions are more or less a 1:1 translation of the x86 vectorized routines.This should resolve issue #2027.