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
