Re: Proposal for enabling auto-vectorization for checksum calculations

From: Oleg Tselebrovskiy <o(dot)tselebrovskiy(at)postgrespro(dot)ru>
To: Andrew Kim <tenistarkim(at)gmail(dot)com>
Cc: John Naylor <johncnaylorls(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Proposal for enabling auto-vectorization for checksum calculations
Date: 2025-10-20 15:05:12
Message-ID: 44d2472a978912ca352eb839a24b2176@postgrespro.ru
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2025-10-20 15:11:41 Re: Should we say "wal_level = logical" instead of "wal_level >= logical"
Previous Message Tom Lane 2025-10-20 14:39:19 Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()