From: "Amonson, Paul D" <paul.d.amonson@intel.com>
To: Andres Freund <andres@anarazel.de>
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"
	<pgsql-hackers@lists.postgresql.org>
Subject: RE: Popcount optimization using AVX512
Thread-Topic: Popcount optimization using AVX512
Thread-Index: AdoNliAXaie8RHrzTICrAiLVKKiYCgAsQKyAALaJTAAAAQ7nAAAAxSWAAAGNHgAAA/mZgAAeFdGAAZKQbBAAAw6egA3NuLcAAAvVM4ACbQuxsAAhlXkAAHPWMbAAAkOrAACYCUjgAAMc3wABvefW4A==
Date: Wed, 21 Feb 2024 17:35:57 +0000
Message-ID: <BL1PR11MB5304F0889AB4EB200AF44565DC572@BL1PR11MB5304.namprd11.prod.outlook.com>
References: <BL1PR11MB5304E4A5AA87D9F0126B4502DC462@BL1PR11MB5304.namprd11.prod.outlook.com>
 <202402071013.reqe3bdnjjuj@alvherre.pgsql>
 <BL1PR11MB53042C623F17F17469B2C053DC4B2@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240209183450.snllkk7erharc73y@awork3.anarazel.de>
 <BL1PR11MB5304F65FD8D19B608F9E8624DC482@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240212203714.u4p7yeu7isuemmze@awork3.anarazel.de>
In-Reply-To: <20240212203714.u4p7yeu7isuemmze@awork3.anarazel.de>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?917Rg5+HZKT+flFPuXk87ituoUFm4d3I7PHKNP71vj6YoMqAo1VqDIGWW0hJ?=
 =?us-ascii?Q?Y3f49U+bQcoMpl+GL85NhIwZda4Xh2OWnzrsOfufc31FKI9NP3LXmOqzxe7K?=
 =?us-ascii?Q?Yw2zp//v6R36DEO3Z5ozeSNPTrTScdN0365HB3l/KAI93Vi8N2j1/G7ReiEW?=
 =?us-ascii?Q?v+wzQZCsgIxNrPJ+kUkOp4Ux+jkxO0KXJ4G2tETSiBlXr2erJbqQfDqZ4N6A?=
 =?us-ascii?Q?CzHgxmL4IpUEstfdRaBxruPI1atfJctboDBRJI/6AnWYVkCU/kc8DyhJrHqh?=
 =?us-ascii?Q?urlcXaC3Evqf6lb1jH51Zh+v8kByX3YKHJEQF4N+phJDRJTNMLCaBpCRSvT2?=
 =?us-ascii?Q?xwmWrNtB1QSen/W5uJp7JnUvyLNd7mSs4j1iYySynGfigu+MAyLhb5glgohY?=
 =?us-ascii?Q?HSk4YuZzoz+qypUWZ+ztFCiIpkqMig28czJEF/TzQlfycPBTsDOp8zoMKqSC?=
 =?us-ascii?Q?q3tVZ0tlYDh//sAlK0hywZ+NcheG9D1SkvlGKbEIQzamDh/2TLCjXzqOx1wd?=
 =?us-ascii?Q?JSE/pE5ZhWerfOIh32Tm9b9X5f6l387J3rTZkx3xLHKuq6F6c8Q3go/Z0Stv?=
 =?us-ascii?Q?yuCLLf+/Kh/cxLaRiINTBcqopLCGBU+WzZKdRmBjDyRNLWdakyyjBVicOR7X?=
 =?us-ascii?Q?DpDHkHh8Ig+rIuzNXOTHrpU7nnpYHKjiLlJVGspf1HwoM5JB690HSQX9Ffo+?=
 =?us-ascii?Q?kTNvzldEM8G4WnOWY9Mf+hOSCx7jLpriXeqiuapMnAazopcKsjFiEuOlUGMS?=
 =?us-ascii?Q?rnms4bwsUt5q5d5WX6k4rRCtWmzGA6Nv9AFZN/LQZM8EaR3ppEEhFwaSJGzq?=
 =?us-ascii?Q?rIuhyCOeOkwKQY1VVkpsJ2b97kwHvRKdm1QyPGqHXdPoRAFQmbC9K3oartxP?=
 =?us-ascii?Q?Xgi8j5SfTxb/SiPhl5A6mouEUCQeflLrYxPXy68m07n9NXSy+R/cuCBoh+eo?=
 =?us-ascii?Q?C9m+/MeG2X9xLAoj80GYxBLvnq2FyxHPp8gebTCOegJF3LuCu5BYjjBoqz7+?=
 =?us-ascii?Q?V65PqPtRPEy0HGlRhvj50ZEkLftpzNKCWUioAn+uBusTmYX5QNxJ+nb+Q/Ky?=
 =?us-ascii?Q?teTxhBx5tOAexOpeHRTol+bn2avz//LDZg90qXlLyeDrL19l3Tfy4USi0zU0?=
 =?us-ascii?Q?MJxsgmcZ7iHhatDNLZj6ob532E3vVVSg07XNwQmVc5PPZ6YfTplSR4LJLzLb?=
 =?us-ascii?Q?dnDOqbmMbI/1dB768BpVpdcF/5Z5Jy6Nm5qWLgoXl3gCaYyREc38qpm2d9Xi?=
 =?us-ascii?Q?L0ZdS9bNgWaVHBxYNyqHWA1LBUK7EQH8qmTHuThIzfa7Ubs9GEn3tyLYM87p?=
 =?us-ascii?Q?1n+Oewvg/eN8+xTgE2fbnMAhLlmi4/H4mARRu7t3NKvTWTm3g6l6PFBQ7JqR?=
 =?us-ascii?Q?sAfWvoSl/ZGQcvNJspnSfoX3FVquMcUsNlWvUb8X+0Hq07QeoGoZiy/DQ68O?=
 =?us-ascii?Q?VK/kqSW1jNymLoO39A0Pr/t2NXZyeaYQ/rgp5hnbCBGU9JZqHGN9McVF02M/?=
 =?us-ascii?Q?UY8tZOW1z3F/TwBsFZxsjoQx9ofpt9WnLlKJbCRO+sqEUDdK2kF2QV8Vdf2l?=
 =?us-ascii?Q?jWfp/I+l4eoAfh4A/mqhFHMyMyqhTXHxWbTR9ANc?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f6eeeaf1-bcd0-4c65-64f4-08dc3303905a
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Feb 2024 17:35:57.9315
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: jqRPxOkxONj9+OmDC9Ma2Pkeu3Zut6lADdVTrnmOphpOxEJlholqDgcKdjSTwilEQuZxGcYy8MSleAQ7yvefwM1a3abGlTmd/Ka9A0gJ3JQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7891
X-OriginatorOrg: intel.com
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/BL1PR11MB5304F0889AB4EB200AF44565DC572%40BL1PR11MB5304.namprd11.prod.outlook.com>
Precedence: bulk

Hi,

I am encountering a problem that I don't think I understand. I cannot get t=
he MSVC build to link in CI. I added 2 files to the build, but the linker i=
s complaining about the original pg_bitutils.c file is missing (specificall=
y 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 li=
bs 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>=20
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.shank=
aran@intel.com>; Nathan Bossart <nathandbossart@gmail.com>; Noah Misch <noa=
h@leadboat.com>; Tom Lane <tgl@sss.pgh.pa.us>; Matthias van de Meent <boeke=
wurm+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 remov=
e the AVX-512 popcount feature from MSVC builds. Sound ok?

CI [1], whould be able to test at least building. Including via cfbot, auto=
matically run for each commitfest entry - you can see prior runs at [2].  T=
hey 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 consi=
dered supported at configure time:

...
[00:23:48.480] Checking if "__get_cpuid" : links: NO [00:23:48.480] Checkin=
g 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 th=
e 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 cor=
rect, 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=20
> pointers here would be helpful.

Should be fairly simple, add it to the replace_funcs_pos and add the releva=
nt 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 exis=
ting 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=20
> 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_bit=
utils.c (entry point +sw popcnt implementation), pg_popcnt_choose.c (CPUID =
and xgetbv check) and pg_popcnt_x86_64_accel.c (64/512bit x86 implementatio=
ns).
> I'm not an expert in meson, but splitting might add complexity to meson.b=
uild.
>
> Could you elaborate if there are other benefits to the split file approac=
h?

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%2F4=
7%2F4675
[3] https://cirrus-ci.com/task/5645112189911040