Thanks for the quick reply Bruno, updated patch attached On Mon, 16 Dec 2024 at 17:25, Bruno Haible <br...@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
