public inbox for [email protected]
help / color / mirror / Atom feedFrom: Oleg Tselebrovskiy <[email protected]>
To: Andrew Kim <[email protected]>
Cc: John Naylor <[email protected]>
Cc: [email protected]
Subject: Re: Proposal for enabling auto-vectorization for checksum calculations
Date: Mon, 20 Oct 2025 18:05:12 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAK64mnd6VAtTbar=QS4sbWJpfFxjksjg2ERNkGw1tRwh_kc6mw@mail.gmail.com>
References: <[email protected]>
<CANWCAZYZQw-nzTXbx3Bk332VtY9_D7ksDsuMZ0A-iDZ53yG7Ng@mail.gmail.com>
<CAK64mnfeWLBRbMfnOsag0vGTDnT84KJzpuei40nG0OHyw4SESw@mail.gmail.com>
<CANWCAZa1b2rcvoK657SmcKwh2P2cgASQ1D-0JPj5d3LbfaAVgA@mail.gmail.com>
<CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
<[email protected]>
<CAK64mnd6VAtTbar=QS4sbWJpfFxjksjg2ERNkGw1tRwh_kc6mw@mail.gmail.com>
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
view thread (35+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Proposal for enabling auto-vectorization for checksum calculations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox