Re: Proposal for enabling auto-vectorization for checksum calculations

Greetings!

I've also tried to use AVX2 to speedup checksums and I've found your
approach quite interesting

But I see some issues with v6 patch

1) checksum.c was moved to src/port, but special meson rules are left in
src/backend/storage/page/meson.build. As a result, assembly code for
moved src/port/checksum.c doesn't use -funroll-loops and
-ftree-vectorize (latter isn't probably needed now, due to the nature of
the patch). The same is true for src/port/Makefile, there are no
instructions to use CFLAGS_UNROLL_LOOPS and CFLAGS_VECTORIZE

2) checksum.c was moved to src/port, but checksum.h and checksum_impl.h
are left in src/include/storage. I think they both should be moved to
src/include/port, as John Naylor suggested in his review of v5

3) checksum_impl.h now doesn't provide any code, so including it in
external programs won't allow checksum calculation. I think that all
code should be in checksum_impl.h, and external programs could just
define USE_AVX2_WITH_RUNTIME_CHECK (probably using similar checks as we
are) to use AVX2 implementation. If not - then they will default to
default realisation

4) I don't understand why do we need to check for AVX2 intrinsics if we
don't use those in code (at least I don't see them directly)? As in
review of v5, couldn't test functions in configure, config/c-compiler.m4
and ./meson.build just be {return 0;} or {return 1;}?

5) Why do we need both src/backend/storage/page/checksum.c and
src/port/checksum.c?

6)
> +/* Function declarations for ISA-specific implementations */
> +uint32 pg_checksum_block_default(const PGChecksummablePage *page);
> +#ifdef USE_AVX2_WITH_RUNTIME_CHECK
> +uint32 pg_checksum_block_avx2(const PGChecksummablePage *page);
> +#endif

What is "ISA-specific implementations" in this comment? Maybe I'm just
not familiar with the term? Or is it an artifact from macro
implementation?

7) Why remove all comments from code of pg_checksum_block_default? I
could understand if you just removed comments from
pg_checksum_block_avx2, since it just duplicates code (though I
personally would leave all the comments even when duplicating code), but
I don't understand removing comments from pg_checksum_block_default

8) It might be a personal taste, but pg_checksum_block_dispatch looks
more like "choose" function from src/port/pg_crc32c_sse42_choose.c and
alike. "dispatch" from src/include/port/pg_crc32c looks a little
different - we don't choose function pointer once there, we choose
between inlined computation and calling a function with runtime check.
So I'd suggest changing name of pg_checksum_block_dispatch to
pg_checksum_block_choose

Other than those, I think the core of this patch is good

Oleg Tselebrovskiy, PostgresPro