Subject: RE: Popcount optimization using AVX512

From: "Amonson, Paul D" <[email-masked]>
Date: Tue, 19 Mar 2024 22:56:01 +0000

> -----Original Message-----
> From: Nathan Bossart <[email-masked]>
>
> Committed.  Thanks for the suggestion and for reviewing!
> 
> Paul, I suspect your patches will need to be rebased after commit cc4826d.
> Would you mind doing so?

Changed in this patch set.

* Rebased.
* Direct *slow* calls via macros as shown in example patch.
* Changed the choose filename to be platform specific as suggested.
* Falls back to intermediate "Fast" methods if AVX512 is not available at runtime.
* inline used where is makes sense, remember using "extern" negates "inline".
* Fixed comment issues pointed out in review.

I tested building with and without TRY_POPCOUNT_FAST, for both configure and meson build systems, and ran in CI.

Thanks,
Paul

---

From: David Rowley <[email-masked]>
Date: Wed, 20 Mar 2024 17:26:07 +1300

> Changed in this patch set.

Thanks for rebasing.

I don't think there's any need to mention Intel in each of the
following comments:

+# Check for Intel AVX512 intrinsics to do POPCNT calculations.

+# Newer Intel processors can use AVX-512 POPCNT Capabilities (01/30/2024)

AMD's Zen4 also has AVX512, so it's misleading to indicate it's an
Intel only instruction.  Also, writing the date isn't necessary as we
have "git blame"

David

---

From: "Amonson, Paul D" <[email-masked]>
Date: Wed, 20 Mar 2024 14:23:55 +0000

> From: David Rowley <[email-masked]>
> Sent: Tuesday, March 19, 2024 9:26 PM
> To: Amonson, Paul D <[email-masked]>
> 
> AMD's Zen4 also has AVX512, so it's misleading to indicate it's an Intel only
> instruction.  Also, writing the date isn't necessary as we have "git blame"

Fixed.

Thanks,
Paul

---

From: David Rowley <[email-masked]>
Date: Thu, 21 Mar 2024 13:28:16 +1300

> * inline used where is makes sense, remember using "extern" negates "inline".

I'm not sure about this "extern negates inline" comment.  It seems to
me the compiler is perfectly free to inline a static function into an
external function and it's free to inline the static function
elsewhere within the same .c file.

The final sentence of the following comment that the 0001 patch
removes explains this:

/*
 * When the POPCNT instruction is not available, there's no point in using
 * function pointers to vary the implementation between the fast and slow
 * method.  We instead just make these actual external functions when
 * TRY_POPCNT_FAST is not defined.  The compiler should be able to inline
 * the slow versions here.
 */

Also, have a look at [1].  You'll see f_slow() wasn't even compiled
and the code was just inlined into f().  I just added the
__attribute__((noinline)) so that usage() wouldn't just perform
constant folding and just return 6.

I think, unless you have evidence that some common compiler isn't
inlining the static into the extern then we shouldn't add the macros.
It adds quite a bit of churn to the patch and will break out of core
code as you no longer have functions named pg_popcount32(),
pg_popcount64() and pg_popcount().

David

[1] https://godbolt.org/z/6joExb79d

---

From: "Amonson, Paul D" <[email-masked]>
Date: Thu, 21 Mar 2024 19:17:54 +0000

> Sent: Wednesday, March 20, 2024 5:28 PM
> Cc: Nathan Bossart <[email-masked]>; Andres Freund
>
> I'm not sure about this "extern negates inline" comment.  It seems to me the
> compiler is perfectly free to inline a static function into an external function
> and it's free to inline the static function elsewhere within the same .c file.
> 
> The final sentence of the following comment that the 0001 patch removes
> explains this:
> 
> /*
> Also, have a look at [1].  You'll see f_slow() wasn't even compiled and the code
> was just inlined into f().  I just added the
> __attribute__((noinline)) so that usage() wouldn't just perform constant
> folding and just return 6.
> 
> I think, unless you have evidence that some common compiler isn't inlining the
> static into the extern then we shouldn't add the macros.
> It adds quite a bit of churn to the patch and will break out of core code as you
> no longer have functions named pg_popcount32(),

This may be a simple misunderstanding extern != static. If I use the "extern" keyword then a symbol *will* be generated and inline will be ignored. This is NOT true of "static inline", where the compiler will try to inline the method. :)

In this patch set:
* I removed the macro implementation.
* Made everything that could possibly be inlined marked with the "static inline" keyword.
* Conditionally made the *_slow() functions "static inline" when TRY_POPCONT_FAST is not set.
* Found and fixed some whitespace errors in the AVX code implementation.

Thanks,
Paul

---

From: "Amonson, Paul D" <[email-masked]>
Date: Mon, 25 Mar 2024 15:06:16 +0000

> From: Amonson, Paul D <[email-masked]>
> Sent: Thursday, March 21, 2024 12:18 PM
> To: David Rowley <[email-masked]>

I am re-posting the patches as CI for Mac failed (CI error not code/test error). The patches are the same as last time.

Thanks,
Paul

---

From: Tom Lane <[email-masked]>
Date: Mon, 25 Mar 2024 11:12:02 -0400

> I am re-posting the patches as CI for Mac failed (CI error not code/test error). The patches are the same as last time.

Just for a note --- the cfbot will re-test existing patches every
so often without needing a bump.  The current cycle period seems to
be about two days.

			regards, tom lane

---

From: "Amonson, Paul D" <[email-masked]>
Date: Mon, 25 Mar 2024 15:20:19 +0000

> From: Tom Lane <[email-masked]>
> Sent: Monday, March 25, 2024 8:12 AM
> Cc: David Rowley <[email-masked]>; Nathan Bossart
> Subject: Re: Popcount optimization using AVX512
>...
> Just for a note --- the cfbot will re-test existing patches every so often without
> needing a bump.  The current cycle period seems to be about two days.
> 
> 			regards, tom lane

Good to know! Maybe this is why I thought it originally passed CI and suddenly this morning there is a failure. I noticed at least 2 other patch runs also failed in the same way.

Thanks,
Paul

---

From: Joe Conway <[email-masked]>
Date: Mon, 25 Mar 2024 11:44:34 -0400

> so often without needing a bump.  The current cycle period seems to

Just an FYI -- there seems to be an issue with all three of the macos 
cfbot runners (mine included). I spent time over the weekend working 
with Thomas Munro (added to CC list) trying different fixes to no avail. 
Help from macos CI wizards would be gratefully accepted...

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Amonson, Paul D" <[email-masked]>
Date: Mon, 25 Mar 2024 18:42:36 +0000

> Sent: Monday, March 25, 2024 8:20 AM
> To: Tom Lane <[email-masked]>
> <[email-masked]>; Andres Freund <[email-masked]>; Alvaro
> Herrera <[email-masked]>; Shankaran, Akash
> <[email-masked]>; Noah Misch <[email-masked]>; Matthias
> van de Meent <[email-masked]>; pgsql-
> [email-masked]
> Subject: RE: Popcount optimization using AVX512
>

Ok, CI turned green after my re-post of the patches.  Can this please get merged?

Thanks,
Paul

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 25 Mar 2024 15:05:51 -0500

> Ok, CI turned green after my re-post of the patches.  Can this please get

Thanks for the new patches.  I intend to take another look soon.

-- 
Nathan Bossart