RE: Proposal for Updating CRC32C with AVX-512 Algorithm.

> > 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(dot)d(dot)amonson(at)intel(dot)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 <nathan(at)postgresql(dot)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