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

Thanks for the new patch version!

Another round of review:

1) I think that changes to contrib/pageinspect/rawpage.c should be in 
the main patch, not in the benchmark patch. Also, without those chages 
the main patch can't compile using make world-bin

2) I still don't get why you check for working intrinsics in configure, 
config/c-compiler.m4 and meson.build, if your patch later uses them. 
I've gotten correct assembly code with this avx2_test function:
     #include <stdint.h>
     #if defined(__has_attribute) && __has_attribute (target)
     __attribute__((target("avx2")))
     static int avx2_test(void)
     {
        return 0;
     }
     #endif
Please, check if this works for you and consider using something similar

3) __builtin_cpu_supports doesn't work on Windows at all. We still have 
to use approach with __get_cpuid

4) Looks like you can safely remove "port/checksum_impl.h" from 
src/bin/pg_checksums/pg_checksums.c. It probably links with libpgport 
and/or libpgcommon, so it gets pg_checksum_page from there. Same with 
src/bin/pg_upgrade/file.c. Maybe those includes are "for clarity" and 
you don't need to remove them, but pg_checksums and pg_upgrade seem to 
work without them

5) You don't need #include <string.h> /* for memcpy */ in 
checksum_impl.h. At the very least, memcpy was used before your patch 
without string.h

6) Why did you remove Assert(sizeof(PGChecksummablePage) == BLCKSZ)? Is 
it always false?

7) Is reformatted variable declaration in pg_checksum_block_default_impl 
really needed? Is there a good reason for it? Or is it auto-formatting 
programm output?

8) Your patch removes one whitespace in this line - for (i = 0; i < 
(uint32)(BLCKSZ / (sizeof(uint32) * N_SUMS)); i++)
If you wish to fix formatting like that - please, do it in a separate 
patch. If this was done automatically by some formatting tool - please, 
revert this change

9) Unneeded empty string
     #define FNV_PRIME 16777619

     +
      /* Use a union so that this code is valid under strict aliasing */
      typedef union

10) You need one line with just /* at the beginning of the comment, look 
at other multiline comments in this file
+    /* For now, AVX2 implementation is identical to default
+     * The compiler will auto-vectorize this with proper flags
+     * Future versions could use explicit AVX2 intrinsics here
       */

11) Function pg_checksum_block_simple isn't used at all.

12) Why do you need those?
+#ifndef PG_CHECKSUM_EXTERNAL_INTERFACE
+#define PG_CHECKSUM_EXTERNAL_INTERFACE

13) Object files are added according to alphabetical order, not logical 
order (src/port/Makefile)
      pg_popcount_aarch64.o \
      pg_popcount_avx512.o \
+    checksum.o \
      pg_strong_random.o \
      pgcheckdir.o \

14) I still think that src/port/checksum.c needs to just include 
src/include/port/checksum_impl.h and have no other logic to keep 
checksum_impl.h's role as "header with full implementation"
Now checksum_impl.h doesn't have any mention of pg_checksum_page

15) Assembly for pg_checksum_block_choose now has full code of 
pg_checksum_block_default. This is probably a result of using inline 
functions
Don't know if this is bad, but it is at least strange

Also, some CFBot checks have failed. Two of them with this error/warning
   checksum.c:88:1: error: no previous prototype for ‘pg_checksum_page’ 
[-Werror=missing-prototypes]
      88 | pg_checksum_page(char *page, BlockNumber blkno)
         | ^~~~~~~~~~~~~~~~
Please, address those

Oleg Tselebrovskiy, PostgresPro