RE: Popcount optimization using AVX512

Hi,

I am encountering a problem that I don't think I understand. I cannot get the MSVC build to link in CI. I added 2 files
tothe build, but the linker is complaining about the original pg_bitutils.c file is missing (specifically symbol
'pg_popcount').To my knowledge my changes did not change linking for the offending file and I see the compiles for
pg_bitutils.cin all 3 libs in the build. All other builds are compiling. 

Any help on this issue would be greatly appreciated.

My fork is at https://github.com/paul-amonson/postgresql/tree/popcnt_patch and the CI build is at
https://cirrus-ci.com/task/4927666021728256.

Thanks,
Paul

-----Original Message-----
From: Andres Freund <andres@anarazel.de>
Sent: Monday, February 12, 2024 12:37 PM
To: Amonson, Paul D <paul.d.amonson@intel.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Shankaran, Akash <akash.shankaran@intel.com>; Nathan Bossart
<nathandbossart@gmail.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 
Subject: Re: Popcount optimization using AVX512

Hi,

On 2024-02-12 20:14:06 +0000, Amonson, Paul D wrote:
> > > +# Check for header immintrin.h
> > ...
> > Do these all actually have to link?  Invoking the linker is slow.
> > I think you might be able to just use cc.has_header_symbol().
>
> I took this to mean the last of the 3 new blocks.

Yep.


> > Does this work with msvc?
>
> I think it will work but I have no way to validate it. I propose we remove the AVX-512 popcount feature from MSVC
builds.Sound ok? 

CI [1], whould be able to test at least building. Including via cfbot, automatically run for each commitfest entry -
youcan see prior runs at [2].  They run on Zen 3 epyc instances, so unfortunately runtime won't be tested.  If you look
at[3], you can see that currently it doesn't seem to be considered supported at configure time: 

...
[00:23:48.480] Checking if "__get_cpuid" : links: NO [00:23:48.480] Checking if "__cpuid" : links: YES ...
[00:23:48.492] Checking if "x86_64: popcntq instruction" compiles: NO ...

Unfortunately CI currently is configured to not upload the build logs if the build succeeds, so we don't have enough
detailsto see why. 


> > This will build all of pgport with the avx flags, which wouldn't be correct, I think? The compiler might inject
automaticuses of avx512 in places, which would cause problems, no? 
>
> This will take me some time to learn how to do this in meson. Any
> pointers here would be helpful.

Should be fairly simple, add it to the replace_funcs_pos and add the relevant cflags to pgport_cflags, similar to how
it'sdone for crc. 


> > While you don't do the same for make, isn't even just using the avx512 for all of pg_bitutils.c broken for exactly
thatreson? That's why the existing code builds the files for various crc variants as their own file. 
>
> I don't think its broken, nothing else in pg_bitutils.c will make use
> of
> AVX-512

You can't really guarantee that compiler auto-vectorization won't decide to do so, no? I wouldn't call it likely, but
it'salso hard to be sure it won't happen at some point. 


> If splitting still makes sense, I propose splitting into 3 files:  pg_bitutils.c (entry point +sw popcnt
implementation),pg_popcnt_choose.c (CPUID and xgetbv check) and pg_popcnt_x86_64_accel.c (64/512bit x86
implementations).
> I'm not an expert in meson, but splitting might add complexity to meson.build.
>
> Could you elaborate if there are other benefits to the split file approach?

It won't lead to SIGILLs ;)

Greetings,

Andres Freund


[1] https://github.com/postgres/postgres/blob/master/src/tools/ci/README
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F47%2F4675
[3] https://cirrus-ci.com/task/5645112189911040