Re: [PATCH] crc: Add PCLMUL implementation

[Sam Russell]

Thanks for the quick reply Bruno, updated patch attached

On Mon, 16 Dec 2024 at 17:25, Bruno Haible <bruno@clisp.org> wrote:

Sam Russell wrote:
> crc-x86_64 is added as its own package with minimal bindings in crc.c to
> support, flagged off by the GL_CRC_PCLMUL flag which is only set if the
> crc-x86_64 package is selected.

Thanks. Some final nitpicking from my side:

In crc-x86_64-pclmul.c:
> +#include <config.h>
> +
> +#include "x86intrin.h"
> +#include "crc.h"
> +#include "crc-x86_64.h"

Make it a habit to include the specification header, in this case
"crc-x86_64.h", as the first one after <config.h>. This will help detecting
if the header file is not self-contained (i.e. depends on some typedefs from
other include files).

> +crc32_update_no_xor_pclmul (uint32_t crc, const void *buf, size_t len)
> +  {

You can unindent the entire function block by 2 spaces.

> +    while (bytes_remaining >= 32) {

GNU coding style:

       while (bytes_remaining >= 32)
         {

> +    if (bytes_remaining != 16){

GNU coding style:

       if (bytes_remaining != 16)
         {

> +    }
> +    else {

GNU coding style:

         }
       else
         {

In crc.c:
> +      pclmul_enabled = (0 < __builtin_cpu_supports ("pclmul") &&
> +                        0 < __builtin_cpu_supports ("avx"));

GNU coding style:

         pclmul_enabled = (0 < __builtin_cpu_supports ("pclmul")
                           && 0 < __builtin_cpu_supports ("avx"));

(twice)

In crc-x86_64.m4:
> +  AC_MSG_CHECKING([if pclmul intrinsic exists])
> +  AC_CACHE_VAL([gl_cv_crc_pclmul],[
...
> +  AC_MSG_RESULT([$gl_cv_crc_pclmul])

The triple AC_MSG_CHECKING, AC_CACHE_VAL, AC_MSG_RESULT can be written in
more concise way with AC_CACHE_CHECK.

> +  AM_CONDITIONAL([GL_CRC_PCLMUL],
> +                [test $gl_cv_crc_pclmul = yes])

This conditional is currently ignored. But AFAICS, if you compile the package
on an x86_64 CPU without pclmul feature, crc-x86_64-pclmul.c will be compiled
anyway and produce an error at the line
  #pragma GCC target("pclmul,avx")
right? I think the best way to fix this is to change, in the module description,

  lib_SOURCES += crc-x86_64-pclmul.c

to

  if GL_CRC_PCLMUL
  lib_SOURCES += crc-x86_64-pclmul.c
  endif

Bruno

0001-crc-Add-PCLMUL-implementation.patch
Description: Binary data