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
