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
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 ? |