Re: [PATCH] crc: Add PCLMUL implementation

[Simon Josefsson]

Sam Russell <sam.h.russell@gmail.com> writes:

> Thanks for the quick reply Bruno, updated patch attached

I really like what you did here!  It is clean and easy to review,
separated from the core crc.c code.  In retrospect the slice-by-8
variant should have followed this pattern instead, but I won't pester
you with that request unless you want to clean it up :)

I'm now ready to push when these nits are corrected:

> Subject: [PATCH] crc: Add PCLMUL implementation
>
> * lib/crc-x86_64-pclmul.c: Implement CRC32 with PCLMUL intrinsics
> * lib/crc-x86_64.h: Add header for CRC32 with PCLMUL instrinsics
> * lib/crc.c: Use PCLMUL implementation if available
> * m4/crc-x86_64.m4: Detect PCLMUL and build accordingly
> * modules/crc-x86_64: New module for crc-x86_64

End sentences with '.'

> +++ b/ChangeLog
> @@ -1,3 +1,13 @@
> +2024-12-16  Sam Russell  <sam.h.russell@gmail.com>
> +
> +    crc: Add PCLMUL implementation
> +
> +    * lib/crc-x86_64-pclmul.c: Implement CRC32 with PCLMUL intrinsics
> +     * lib/crc-x86_64.h: Add header for CRC32 with PCLMUL instrinsics
> +    * lib/crc.c: Use PCLMUL implementation if available
> +    * m4/crc-x86_64.m4: Detect PCLMUL and build accordingly
> +    * modules/crc-x86_64: New module for crc-x86_64

Same here.

> +++ b/lib/crc-x86_64-pclmul.c
> @@ -0,0 +1,203 @@
> +/* crc-x86_64.c -- CRC32 implementation using SSE/AVX1

That filename comment is now wrong.

> +   Copyright (C) 2005-2006, 2009-2024 Free Software Foundation, Inc.

Is that true?  If this new original code, shouldn't it say 2024?  If
this isn't new original code (does it come from coreutils), it make
sense to add a comment saying where it is coming from, like:

/* Code coming from GNU CoreUtils 9.5. */

> +  __m128i in256[4] = {0};
...
> +  __m128i in1 = _mm_setzero_si128();

Could you run 'indent' on this file?  I think these should be '{ 0 }'
and '...si128 ();' respectively.

> +++ b/lib/crc-x86_64.h
> @@ -0,0 +1,37 @@
> +/* crc-x86_64.h -- CRC32 implementation using SSE/AVX1

Shouldn't this be crc-x86_64-pclmul.h now?

> +   Copyright (C) 2005-2006, 2009-2024 Free Software Foundation, Inc.

Same as above.

> +++ b/m4/crc-x86_64.m4
> @@ -0,0 +1,39 @@
> +# crc-x86_64.m4

crc-x86_64-pclmul.m4?

> +AC_DEFUN([gl_CRC_PCLMUL],

Add _X86_64_PCLMUL?

> +  CFLAGS="-mavx -mpclmul $CFLAGS"
> +  AC_CACHE_CHECK([if pclmul intrinsic exists], [gl_cv_crc_pclmul],[

I'm unsure about those CFLAGS.  Aren't these CFLAGS required when
building the code too?  If not, why can't the same pattern to detect
PCLMUL during ./configure which is done during run-time?

/Simon

signature.asc
Description: PGP signature