Re: Use __attribute__((target(sse4.2))) for SSE42 CRC32C

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: "Devulapalli, Raghuveer" <raghuveer(dot)devulapalli(at)intel(dot)com>
Cc: "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Use __attribute__((target(sse4.2))) for SSE42 CRC32C
Date: 2024-11-08 17:31:17
Message-ID: Zy5K5Qmlb3Z4dsd4@nathan
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

- prog = '''
+ sse42_crc_prog = '''

nitpick: Why does this need to be renamed?

+#ifdef TEST_SSE42_WITH_ATTRIBUTE
+#if defined(__has_attribute) && __has_attribute (target)
+__attribute__((target("sse4.2")))
+#endif
+#endif

So, for meson builds, we do test with and without
__attribute___((target(..."))), but for autoconf builds, we check for
__SSE4_2__ to determine whether we need a runtime check. This difference
isn't the fault of your patch, but it's a little odd. That being said, I'm
not sure there's a problem with either approach.

+if host_cpu == 'x86' or host_cpu == 'x86_64'
+ pgport_sources += files(
+ 'pg_crc32c_sse42_choose.c',
+ 'pg_crc32c_sse42.c',
+ )
+endi

We could probably just build these unconditionally (i.e., put them in the
first list of pgport_sources in this file) instead of keeping this
complexity in the build scripts.

+#if defined(USE_SSE42_CRC32C) || defined(USE_SSE42_CRC32C_WITH_RUNTIME_CHECK)

Can we move this to just below #include "c.h"? The reason I didn't do this
for the AVX-512 stuff is because TRY_POPCNT_FAST is defined in
pg_bitutils.h, but looking again, I may be able to at least move the check
for USE_AVX512_POPCNT_WITH_RUNTIME_CHECK up similarly.

--
nathan

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bertrand Drouvot 2024-11-08 17:33:52 Re: define pg_structiszero(addr, s, r)
Previous Message Dmitry Dolgov 2024-11-08 16:43:06 Re: Changing shared_buffers without restart