Re: [PATCH] Add native windows on arm64 support

On 17/01/2023 22:51, Andres Freund wrote:
> Hi,
>
> On 2022-12-16 10:52:23 +0000, Niyas Sait wrote:
>> Subject: [PATCH v7] Enable postgres native build for windows-arm64 platform
>
>> elif host_cpu == 'arm' or host_cpu == 'aarch64'
>>
>> - prog = '''
>> + if cc.get_id() == 'msvc'
>> + cdata.set('USE_ARMV8_CRC32C', false)
>> + cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
>> + have_optimized_crc = true
>
> I dimly recall that windows might actually require the relevant extension on
> arm?

Do you mean we don't need the runtime checks for CRC ?

CRC is an optional extension for ARMv8 base architecture. I am not sure
if windows make it mandatory to have this implementation.

>
>> + else
>> + prog = '''
>> #include <arm_acle.h>
>
> I'd just make this include #ifdef _MSV_VER (or whatever it is).

The code snippet is not used for MSVC part. I am not sure why we need to
add the #ifdef _MSC_VER.

>> int main(void)
>> @@ -1960,18 +1966,19 @@ int main(void)
>> }
>> '''
>>
>> - if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
>> - args: test_c_args)
>> - # Use ARM CRC Extension unconditionally
>> - cdata.set('USE_ARMV8_CRC32C', 1)
>> - have_optimized_crc = true
>> - elif cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd with -march=armv8-a+crc',
>> - args: test_c_args + ['-march=armv8-a+crc'])
>> - # Use ARM CRC Extension, with runtime check
>> - cflags_crc += '-march=armv8-a+crc'
>> - cdata.set('USE_ARMV8_CRC32C', false)
>> - cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
>> - have_optimized_crc = true
>> + if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
>> + args: test_c_args)
>
> Seems like it'd be easier to read if you don't re-indent this, but just have
> the cc.get_id() == 'msvc' part of this if/else-if.
>

Yes that looks better. will do it in next patch.

Thanks Andres for the review.

--
Niyas