> > create mode 100644 src/test/modules/test_crc32c/test_crc32c.c > > create mode 100644 src/test/modules/test_crc32c/test_crc32c.control > > Needs to be integrated with the meson based build as well.
Done.
> > +drive_crc32c(PG_FUNCTION_ARGS)
> > +{
> > + int64 count = PG_GETARG_INT64(0);
> > + int64 num = PG_GETARG_INT64(1);
> > + pg_crc32c crc = 0xFFFFFFFF;
> > + const char* data = malloc((size_t)num);
>
> This is computing a crc of uninitialized data. That's
> a) undefined behaviour
> b) means the return value is basically random
> c) often will just CRC a lot of zeroes
Good point. I added random data to the buffer before computing the crc value
and verified that this didn't affect the benchmark numbers.
> > From da26645ec8515e0e6d91e2311a83c3bb6649017e Mon Sep 17 00:00:00
> 2001
> > From: Paul Amonson <paul.d.amon...@intel.com>
> > Date: Tue, 23 Jul 2024 11:23:23 -0700
> > Subject: [PATCH v6 2/6] Move all HW checks to common file.
>
> Would be good to actually include a justification here.
Added a comment for this.
> > +#include "port/pg_hw_feat_check.h"
> > +
> > +/* Define names for EXX registers to avoid hard to see bugs in code
> > +below. */ typedef unsigned int exx_t; typedef enum {
> > + EAX = 0,
> > + EBX = 1,
> > + ECX = 2,
> > + EDX = 3
> > +} reg_name;
>
> Shouldn't this be in some x86 specific ifdef?
The updated version has the #ifdef x86/x86_64 guard.
> > +undefine([Ac_cachevar])dnl
> > +])# PGAC_AVX512_CRC32_INTRINSICS
> > +
>
> Why is all this stuff needed inside a configure check? We don't need to check
> entire algorithms to check if we can build and link sepcific instructions, no?
Yup, this is unnecessary. I have modified the checks in meson and configure to
keep just couple of instructions to test for _mm512_clmulepi64_epi128
(vpclmulqdq) and _mm_xor_epi64 (avx512vl) instructions only.
> > From a495124ee42cb8f9f206f719b9f2235aff715963 Mon Sep 17 00:00:00 2001
> > From: Nathan Bossart <nat...@postgresql.org>
> > Date: Wed, 16 Oct 2024 15:57:55 -0500
> > Subject: [PATCH v6 5/6] use __attribute__((target(...))) for AVX-512
> > stuff
>
> Huh, so now we're undoing a bunch of stuff done earlier. Makes this series
> pretty
> hard to review.
As Nathan suggested, we moved this to a separate thread. The latest set of
patches here need to applied on top of patches in that thread.
Raghuveer
v7-0001-Add-a-Postgres-SQL-function-for-crc32c-benchmarki.patch
Description: v7-0001-Add-a-Postgres-SQL-function-for-crc32c-benchmarki.patch
v7-0002-Refactor-consolidate-x86-ISA-and-OS-runtime-check.patch
Description: v7-0002-Refactor-consolidate-x86-ISA-and-OS-runtime-check.patch
v7-0003-Add-AVX-512-CRC32C-algorithm-with-a-runtime-check.patch
Description: v7-0003-Add-AVX-512-CRC32C-algorithm-with-a-runtime-check.patch
