Andres, After consulting some Intel internal experts on MSVC the linking issue as it stood was not resolved. Instead, I created a MSVC ONLY work-around. This adds one extra functional call on the Windows builds (The linker resolves a real function just fine but not a function pointer of the same name). This extra latency does not exist on any of the other platforms. I also believe I addressed all issues raised in the previous reviews. The new pg_popcnt_x86_64_accel.c file is now the ONLY file compiled with the AVX512 compiler flags. I added support for the MSVC compiler flag as well. Both meson and autoconf are updated with the new refactor.
I am attaching the new patch. Paul -----Original Message----- From: Amonson, Paul D <paul.d.amon...@intel.com> Sent: Monday, February 26, 2024 9:57 AM To: Amonson, Paul D <paul.d.amon...@intel.com>; Andres Freund <and...@anarazel.de> Cc: Alvaro Herrera <alvhe...@alvh.no-ip.org>; Shankaran, Akash <akash.shanka...@intel.com>; Nathan Bossart <nathandboss...@gmail.com>; Noah Misch <n...@leadboat.com>; Tom Lane <t...@sss.pgh.pa.us>; Matthias van de Meent <boekewurm+postg...@gmail.com>; pgsql-hackers@lists.postgresql.org Subject: RE: Popcount optimization using AVX512 Hello again, This is now a blocking issue. I can find no reason for the failing behavior of the MSVC build. All other languages build fine in CI including the Mac. Since the master branch builds, I assume I changed something critical to linking, but I can't figure out what that would be. Can someone with Windows/MSVC experience help me? * Code: https://github.com/paul-amonson/postgresql/tree/popcnt_patch * CI build: https://cirrus-ci.com/task/4927666021728256 Thanks, Paul -----Original Message----- From: Amonson, Paul D <paul.d.amon...@intel.com> Sent: Wednesday, February 21, 2024 9:36 AM To: Andres Freund <and...@anarazel.de> Cc: Alvaro Herrera <alvhe...@alvh.no-ip.org>; Shankaran, Akash <akash.shanka...@intel.com>; Nathan Bossart <nathandboss...@gmail.com>; Noah Misch <n...@leadboat.com>; Tom Lane <t...@sss.pgh.pa.us>; Matthias van de Meent <boekewurm+postg...@gmail.com>; pgsql-hackers@lists.postgresql.org Subject: 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 to the 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.c in 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 <and...@anarazel.de> Sent: Monday, February 12, 2024 12:37 PM To: Amonson, Paul D <paul.d.amon...@intel.com> Cc: Alvaro Herrera <alvhe...@alvh.no-ip.org>; Shankaran, Akash <akash.shanka...@intel.com>; Nathan Bossart <nathandboss...@gmail.com>; Noah Misch <n...@leadboat.com>; Tom Lane <t...@sss.pgh.pa.us>; Matthias van de Meent <boekewurm+postg...@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 - you can 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 details to see why. > > This will build all of pgport with the avx flags, which wouldn't be > > correct, I think? The compiler might inject automatic uses 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's done 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 that reson? 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's also 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
v5-0001-Add-support-for-AVX512-implemented-POPCNT.patch
Description: v5-0001-Add-support-for-AVX512-implemented-POPCNT.patch
