awslabs / aws-checksums

Cross-Platform HW accelerated CRC32c and CRC32 with fallback to efficient SW implementations. C interface with language bindings for each of our SDKs
Apache License 2.0
53 stars 52 forks source link

AVX512 branch: aws_checksums_crc32c_avx512() computes incorrect checksum #88

Open pbadari opened 5 months ago

pbadari commented 5 months ago

Describe the bug

aws_checksums_crc32c_avx512() is doing an extra bit flipping on the previousCRC causing incorrect checksums

Expected Behavior

aws_checksums_crc32c_avx512() computed checksum matches sse4.2 or sw computed versions.

Current Behavior

aws_checksums_crc32c_hw() has bit flips previousCRC value and has this comment:

/* this is the entry point. We should only do the bit flip once. It should not be done for the subfunctions and
     * branches.*/

    uint32_t crc = ~previousCrc32;

aws_checksums_crc32c_avx512() is a subfunction and it should NOT bit flip again.

Reproduction Steps

Compute crc32c checksum on the same buffer with and without AVX512 code and compare the results.

Possible Solution

diff --git a/source/intel/intrin/crc32c_sse42_avx512.c b/source/intel/intrin/crc32c_sse42_avx512.c
index 837a1ba..a4c7a00 100644
--- a/source/intel/intrin/crc32c_sse42_avx512.c
+++ b/source/intel/intrin/crc32c_sse42_avx512.c
@@ -27,7 +27,7 @@ uint32_t aws_checksums_crc32c_avx512(const uint8_t *input, int length, uint32_t
     AWS_ASSERT(
         length >= 256 && "invariant violated. length must be greater than 255 bytes to use avx512 to compute crc.");

-    uint32_t crc = ~previous_crc;
+    uint32_t crc = previous_crc;
     /*
      * Definitions of the bit-reflected domain constants k1,k2,k3,k4,k5,k6
      * are similar to those given at the end of the paper

Additional Information/Context

No response

aws-checksums version used

AVX512 branch

Compiler and version used

gcc version 11.4.0

Operating System and version

Ubuntu 22.04

jmklix commented 4 months ago

We are working on adding support for AVX512 https://github.com/awslabs/aws-checksums/pull/79 https://github.com/awslabs/aws-checksums/pull/81

pbadari commented 4 months ago

We are working on adding support for AVX512 #79 #81

Right now, there are 2 different AVX512 implementations of CRC32 and CRC32c. One is written by me (in Jonathan's AVX512 branch and #89) and other is written by Marco #90. Both versions perform similar (Marco's version is common across crc32 and crc32c). Please review and let me know which one you prefer. I can provide any help needed to merge them.