Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVX512 branch: aws_checksums_crc32c_avx512() computes incorrect checksum #88

Open
pbadari opened this issue May 2, 2024 · 2 comments
Open
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue

Comments

@pbadari
Copy link
Contributor

pbadari commented May 2, 2024

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

@pbadari pbadari added the bug We can reproduce the issue and confirmed it is a bug. label May 2, 2024
@pbadari pbadari changed the title AVX512 branch: aws_checksums_crc32c_avx512() computes incorrect checksums AVX512 branch: aws_checksums_crc32c_avx512() computes incorrect checksum May 2, 2024
@jmklix
Copy link
Member

jmklix commented Jun 3, 2024

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

@jmklix jmklix added feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue and removed bug We can reproduce the issue and confirmed it is a bug. labels Jun 3, 2024
@pbadari
Copy link
Contributor Author

pbadari commented Jun 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

2 participants