Horton

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

Devulapalli, Raghuveer <raghuveer.devulapalli@intel.com>

View thread
From: "Devulapalli, Raghuveer" <raghuveer.devulapalli@intel.com>
To: Andres Freund <andres@anarazel.de>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Bruce Momjian <bruce@momjian.us>, Alvaro Herrera <alvherre@alvh.no-ip.org>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, "Shankaran, Akash" <akash.shankaran@intel.com>
Date: 2024-11-25T20:54:48Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Compute CRC32C using AVX-512 instructions where available

Attachments

> >  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.amonson@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 <nathan@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