Date: Wed, 13 Mar 2024 11:39:04 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: "Amonson, Paul D" <paul.d.amonson@intel.com>
Cc: Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"Shankaran, Akash" <akash.shankaran@intel.com>,
	Noah Misch <noah@leadboat.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	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: <20240313163904.GA2560056@nathanxps13>
References: <20240301214457.GA3024365@nathanxps13>
 <BL1PR11MB5304BFE26BAC25624508CFD2DC232@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240304222118.GB3367383@nathanxps13>
 <BL1PR11MB53047C28DDBA6A2E77B9420ADC222@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240305163730.GA3479896@nathanxps13>
 <BL1PR11MB530444AEE08A91242ABA1915DC222@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240307173318.GA455851@nathanxps13>
 <20240307213600.GA563369@nathanxps13>
 <BL1PR11MB5304165E7A81E0107E70B069DC242@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240312013436.GA1787866@nathanxps13>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240312013436.GA1787866@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/20240313163904.GA2560056%40nathanxps13>
Precedence: bulk

A couple of thoughts on v7-0001:

+extern int  pg_popcount32_slow(uint32 word);
+extern int  pg_popcount64_slow(uint64 word);

+/* In pg_popcnt_*_accel source file. */
+extern int  pg_popcount32_fast(uint32 word);
+extern int  pg_popcount64_fast(uint64 word);

Can these prototypes be moved to a header file (maybe pg_bitutils.h)?  It
looks like these are defined twice in the patch, and while I'm not positive
that it's against project policy to declare extern function prototypes in
.c files, it appears to be pretty rare.

+  'pg_popcnt_choose.c',
+  'pg_popcnt_x86_64_accel.c',

I think we want these to be architecture-specific, i.e., only built for
x86_64 if the compiler knows how to use the relevant instructions.  There
is a good chance that we'll want to add similar support for other systems.
The CRC32C files are probably a good reference point for how to do this.

+#ifdef TRY_POPCNT_FAST

IIUC this macro can be set if either 1) the popcntq test in the
autoconf/meson scripts passes or 2) we're building with MSVC on x86_64.  I
wonder if it would be better to move the MSVC/x86_64 check to the
autoconf/meson scripts so that we could avoid surrounding large portions of
the popcount code with this macro.  This might even be a necessary step
towards building these files in an architecture-specific fashion.

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