Re: Proposal for enabling auto-vectorization for checksum calculations - Mailing list pgsql-hackers

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