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

On Thu, Nov 07, 2024 at 09:30:32PM +0000, Devulapalli, Raghuveer wrote:
>> # Check for Intel SSE 4.2 intrinsics to do CRC calculations.
>> #
>> -# First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used -#
>> with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps.
>> CFLAGS_CRC is set to -msse4.2 if that's required.
>> +# Check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used #
>> +with the __attribute__((target("sse4.2"))).
>> PGAC_SSE42_CRC32_INTRINSICS([])
>> -if test x"$pgac_sse42_crc32_intrinsics" != x"yes"; then
>> - PGAC_SSE42_CRC32_INTRINSICS([-msse4.2])
>> -fi
>>
>> IIUC this means we will never set USE_SSE42_CRC32C_WITH_RUNTIME_CHECK.
>
> I don't think so. USE_SSE42_CRC32C additionally requires SSE4_2_TARGETED
> to be true which will only happen when explicitly built with -msse4.2.
> When this explicit compiler flag is missing, we set
> USE_SSE42_CRC32C_WITH_RUNTIME_CHECK to true. This logic is further down
> in the configure.ac file:
>
> if test x"$pgac_sse42_crc32_intrinsics" = x"yes" && test x"$SSE4_2_TARGETED" = x"1" ; then
> USE_SSE42_CRC32C=1
> else

Oh, you are right, sorry.

>> + 'pg_crc32c_sse42_choose.c',
>> + 'pg_crc32c_sse42.c',
>>
>> Can we combine these?
>
> Knowing the AVX-512 will come next, I think it makes sense to keep the
> runtime choose function separate. Otherwise this gets polluted with
> runtime choose function, sse42 algorithm and the avx512 algorithm in the
> next patch. Does that make sense??

Is the idea that we will put both "choose" functions in one file and the
actual CRC-32C code in another? I'm okay with that.

--
nathan