Date: Wed, 31 Jul 2024 16:43:02 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: "Devulapalli, Raghuveer" <raghuveer.devulapalli@intel.com>,
	"Shankaran, Akash" <akash.shankaran@intel.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <dgrowleyml@gmail.com>,
	Ants Aasma <ants.aasma@cybertec.at>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"Amonson, Paul D" <paul.d.amonson@intel.com>,
	Noah Misch <noah@leadboat.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: Popcount optimization using AVX512
Message-ID: <Zqqv5vssUS7vPjT8@nathan>
References: <20240730210701.oyn2wr4zduu2efrg@awork3.anarazel.de>
 <Zqlb1_-BWlzVMbZv@nathan>
 <20240731004959.6ys24432n6xlgemk@awork3.anarazel.de>
 <ZqmRYh3iikm1Kh3D@nathan>
 <20240731014651.byn3exbu6n5v2yqa@awork3.anarazel.de>
 <Zqma-_guYhQmA9_A@nathan>
 <20240731024308.xma56fxidkdydzlo@awork3.anarazel.de>
 <ZqmpHjuDlJSjoZVT@nathan>
 <Zqmrkt6Pyb543bIq@nathan>
 <20240731205254.vfpap7uxwmebqeaf@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240731205254.vfpap7uxwmebqeaf@awork3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/Zqqv5vssUS7vPjT8%40nathan>
Precedence: bulk

On Wed, Jul 31, 2024 at 01:52:54PM -0700, Andres Freund wrote:
> On 2024-07-30 22:12:18 -0500, Nathan Bossart wrote:
>> As I started on this, I remembered why I needed it.  The file
>> pg_popcount_avx512_choose.c is compiled without the AVX-512 flags in order
>> to avoid inadvertently issuing any AVX-512 instructions before determining
>> we have support.  If that's not a concern, we could still probably remove
>> the XSAVE check.
> 
> I think it's a valid concern - but isn't that theoretically also an issue with
> xsave itself? I guess practically the compiler won't do that, because there's
> no practical reason to emit any instructions enabled by -mxsave (in contrast
> to e.g. -mavx, which does trigger gcc to emit different instructions even for
> basic math).

Yeah, this crossed my mind.  It's certainly not the sturdiest of
assumptions...

> I think enabling options like these on a per-translation-unit basis isn't
> really a scalable approach. To actually be safe there could only be a single
> function in each TU and that function could only be called after a cpuid check
> performed in a separate TU. That a) ends up pretty unreadable b) requires
> functions to be implemented in .c files, which we really don't want for some
> of this.

Agreed.

> I think we'd be better off enabling architectural features on a per-function
> basis, roughly like this:
>
> [...]
> 
> /* FIXME: Should be gated by configure check of -mavx512vpopcntdq -mavx512bw support */
> pg_enable_target("avx512vpopcntdq,avx512bw")
> uint64_t
> pg_popcount_avx512(const char *buf, int bytes)
> ...

I remember wondering why the CRC-32C code wasn't already doing something
like this (old compiler versions? non-gcc-like compilers?), and I'm not
sure I ever discovered the reason, so out of an abundance of caution I used
the same approach for AVX-512.  If we can convince ourselves that
__attribute__((target("..."))) is standard enough at this point, +1 for
moving to that.

-- 
nathan