Date: Wed, 13 Mar 2024 13:38:11 -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: <20240313183811.GA2563626@nathanxps13>
References: <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>
 <20240313163904.GA2560056@nathanxps13>
 <BL1PR11MB530495DAB65F71E5F1625F2FDC2A2@BL1PR11MB5304.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BL1PR11MB530495DAB65F71E5F1625F2FDC2A2@BL1PR11MB5304.namprd11.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/20240313183811.GA2563626%40nathanxps13>
Precedence: bulk

On Wed, Mar 13, 2024 at 05:52:14PM +0000, Amonson, Paul D wrote:
>> 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.
> 
> I will look at this for the 'pg_popcnt_x86_64_accel.c' file but the
> 'pg_popcnt_choose.c' file is intended to be for any platform that may
> need accelerators including a possible future ARM accelerator.

I worry that using the same file for *_choose.c for all architectures would
become rather #ifdef heavy.  Since we are already separating out this code
into new files, IMO we might as well try to avoid too many #ifdefs, too.
But this is admittedly less important right now because there's almost no
chance of any new architecture support here for v17.

>> +#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.
> 
> I see the point here; however, this will take some time to get right
> especially since I don't have a Windows box to do compiles on. Should I
> attempt to do this in this patch?

This might also be less important given the absence of any imminent new
architecture support in this area.  I'm okay with it, given we are just
maintaining the status quo.

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