Subject: Re: add AVX2 support to simd.h

From: John Naylor <[email-masked]>
Date: Mon, 1 Jan 2024 19:12:26 +0700

On Thu, Nov 30, 2023 at 12:15 AM Nathan Bossart
<[email-masked]> wrote:
> I don't intend for this patch to be
> seriously considered until we have better support for detecting/compiling
> AVX2 instructions and a buildfarm machine that uses them.

That's completely understandable, yet I'm confused why there is a
commitfest entry for it marked "needs review".

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 2 Jan 2024 10:11:23 -0600

> commitfest entry for it marked "needs review".

Perhaps I was too optimistic about adding support for newer instructions...

I'm tempted to propose that we move forward with this patch as-is after
adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.
There is likely still follow-up work to make these improvements more
accessible, but I'm not sure that is a strict prerequisite here.

(In case it isn't clear, I'm volunteering to set up such a buildfarm
machine.)

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Tom Lane <[email-masked]>
Date: Tue, 02 Jan 2024 12:50:04 -0500

> accessible, but I'm not sure that is a strict prerequisite here.

The patch needs better comments (as in, more than "none whatsoever").
It doesn't need to be much though, perhaps like

+#if defined(__AVX2__)
+
+/*
+ * When compiled with -mavx2 or allied options, we prefer AVX2 instructions.
+ */
+#include <immintrin.h>
+#define USE_AVX2
+typedef __m256i Vector8;
+typedef __m256i Vector32;

Also, do you really want to structure the header so that USE_SSE2
doesn't get defined?  In that case you are committing to provide
an AVX2 replacement every single place that there's USE_SSE2, which
doesn't seem like a great thing to require.  OTOH, maybe there's
no choice given than we need a different definition for Vector8 and
Vector32?

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 2 Jan 2024 16:00:18 -0600

> The patch needs better comments (as in, more than "none whatsoever").

Yes, will do.

> no choice given than we need a different definition for Vector8 and

Yeah, the precedent is to use these abstracted types elsewhere so that any
SIMD-related improvements aren't limited to one architecture.  There are a
couple of places that do explicitly check for USE_NO_SIMD, though.  Maybe
there's an eventual use-case for using SSE2 intrinsics even when you have
AVX2 support, but for now, ensuring we have an AVX2 replacement for
everything doesn't seem particularly burdensome.

---

From: John Naylor <[email-masked]>
Date: Wed, 3 Jan 2024 21:13:52 +0700

> adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.

That means that we would be on the hook to fix it if it breaks, even
though nothing uses it yet in a normal build. I have pending patches
that will break, or get broken by, this, so minus-many from me until
there is an availability story.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 3 Jan 2024 09:29:54 -0600

> there is an availability story.

How will this break your patches?  Is it just a matter of adding more AVX2
support, or something else?

If the requirement is that normal builds use AVX2, then I fear we will be
waiting a long time.  IIUC the current proposals (building multiple
binaries or adding a configuration option that maps to compiler flags)
would still be opt-in, and I'm not sure we can mandate AVX2 support for all
x86_64 builds anytime soon.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 4 Jan 2024 11:48:48 -0600

> (In case it isn't clear, I'm volunteering to set up such a buildfarm

I set up "akepa" to run with -march=x86-64-v3.

---

From: John Naylor <[email-masked]>
Date: Fri, 5 Jan 2024 09:03:39 +0700

On Wed, Jan 3, 2024 at 10:29 PM Nathan Bossart <[email-masked]> wrote:
> would still be opt-in,

If and when we get one of those, I would consider that  a "normal"
build. Since there are no concrete proposals yet, I'm still waiting
for you to justify imposing an immediate maintenance cost for zero
benefit.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 5 Jan 2024 11:04:27 -0600

> for you to justify imposing an immediate maintenance cost for zero

I've been thinking about the configuration option approach.  ISTM that
would be the most feasible strategy, at least for v17.  A couple things
come to mind:

* This option would simply map to existing compiler flags.  We already have
  ways to provide those (-Dc_args in meson, CFLAGS in autoconf).  Perhaps
  we'd want to provide our own shorthand for certain platforms (e.g., ARM),
  but that will still just be shorthand for compiler flags.

* Such an option would itself generate some maintenance cost.  That could
  be worth it because it formalizes the Postgres support for those options,
  but it's still one more thing to track.

Another related option could be to simply document that we have support for
some newer instructions that can be enabled by setting the aforementioned
compiler flags.  That's perhaps a little less user-friendly, but it'd avoid
the duplication and possibly reduce the maintenance cost.  I also wonder if
it'd help prevent confusion when CFLAGS and this extra option conflict.