Re: [PATCH] cksum: Use pclmul hardware instruction for CRC32 calculation

[Kristoffer Brånemyr]

Hi,
Ok, good to know it's progressing!
I think that it would be best not to compile the main cksum.c with the -mavx .. 
flags. With those flags I think the compiler is free to insert AVX instructions 
in the main cksum.o file, which would fail on a CPU not supporting it. Think 
for instance of a package maintainer of a linux distributions, the compiler 
that person used would support -mavx so the flag is enabled by autoconf, but 
another person using the resulting binary might have a CPU not supporting AVX. 
That's what I tried to avoid by only having the cksum_pclmul.c enabling the 
compiler flags, plus runtime detection.

-- 
/Kristoffer Brånemyr 

    Den lördag 13 mars 2021 17:13:46 CET, Pádraig Brady <p@draigbrady.com> 
skrev:  
 
 On 12/03/2021 15:33, Kristoffer Brånemyr wrote:
> Hi,
> 
> I was just wondering if you are planning to merge the change, or if you 
> decided against it? :)
> I wanted to use the cpuid.h autoconf detection for another patch I'm working 
> on.

We're still planning to include it.
I'm making a few adjustments.

The initial one is:

diff --git a/src/local.mk b/src/local.mk
index f54a96c9c..d4a00a1a4 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -358,10 +358,11 @@ src___SOURCES = src/lbracket.c
  nodist_src_coreutils_SOURCES = src/coreutils.h
  src_coreutils_SOURCES = src/coreutils.c

-src/cksum_pclmul.o: CFLAGS += -mavx -mpclmul
  src_cksum_SOURCES = src/cksum.c src/cksum.h
+src_cksum_CFLAGS = $(AM_CFLAGS)
  if USE_PCLMUL_CRC32
  src_cksum_SOURCES += src/cksum_pclmul.c
+src_cksum_CFLAGS += -mavx -mpclmul
  endif
  src_cp_SOURCES = src/cp.c $(copy_sources) $(selinux_sources)
  src_dir_SOURCES = src/ls.c src/ls-dir.c

The above changes from relying on make(1) to append to the variable,
to instead leveraging automake's append support for better portability.
The original used a "target-specific variable", which seems to be GNU specific:
https://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html

To confirm I tried a trivial Makefile on Solaris and FreeBSD,
which gave the following errors respectively:
  "Macro assignment on dependency line"
  "don't know how to make CFLAGS"

Another issue with the original is that it modifies CFLAGS,
which is a user variable, and is advised against.
If the user overrides CFLAGS for a particular make invocation
(which is a common thing I do myself for debugging etc.),
then that would result in a compile failure due to
the necessary flags not being defined. More details at:
https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html

A caveat of the new approach is that the main cksum module
also has these flags applied, but that should be fine.
For completeness if you really wanted to restrict the flags
to a particular module, you could define a noinst_LIBRARY as described at:
https://www.gnu.org/software/automake/manual/html_node/Per_002dObject-Flags.html

The other changes are mainly scope changes etc.
as identified with `make syntax-check`.

cheers,
Pádraig