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

From: "Devulapalli, Raghuveer" <raghuveer(dot)devulapalli(at)intel(dot)com>
To: Nathan Bossart <nathandbossart(at)gmail(dot)com>
Cc: "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "Shankaran, Akash" <akash(dot)shankaran(at)intel(dot)com>
Subject: RE: Use __attribute__((target(sse4.2))) for SSE42 CRC32C
Date: 2024-11-08 17:52:23
Message-ID: PH8PR11MB8286A700203AAE91C5AFCB18FB5D2@PH8PR11MB8286.namprd11.prod.outlook.com
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?

Doesn't need to be renamed, but just a better name to describe that the code is meant for. It will be followed up with avx512_crc_prog in the next patch.

>
> +#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.

I just realized, isn't this a problem on MSVC? When building with MSVC, USE_SSE42_CRC32C is always set to true (because MSVC doesn't require a specific SSE42 flag to build with): see https://gcc.godbolt.org/z/eoc1Ec33j. This means it is always running the SSE42 without a runtime check which can technically SEGILL on a really old CPU (SSE42 is nearly 18 years old though). This problem exists in the master branch too.

>
> +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.

Sure.

> +#if defined(USE_SSE42_CRC32C) ||
> +defined(USE_SSE42_CRC32C_WITH_RUNTIME_CHECK)
>
> Can we move this to just below #include "c.h"?

Sounds good.

Raghuveer

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2024-11-08 18:07:45 Re: Use __attribute__((target(sse4.2))) for SSE42 CRC32C
Previous Message Nathan Bossart 2024-11-08 17:44:24 Re: New GUC autovacuum_max_threshold ?