Message-ID: <44f6c1b6-d712-b69b-82f2-8c525f59aeff@linaro.org>
Date: Thu, 19 Jan 2023 10:09:01 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: [PATCH] Add native windows on arm64 support
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>,
 Ian Lawrence Barwick <barwick@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Thomas Munro <thomas.munro@gmail.com>, Julien Rouhaud <rjuju123@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <20221105183136.hfxyl5dclxdcoyih@awork3.anarazel.de>
 <Y2ishWF0ZkD2JIpv@paquier.xyz>
 <7ff7c416-fdfe-517f-a65c-9e5bd8ff8cec@linaro.org>
 <f40835e6-db1e-3f51-001a-8b911245665d@linaro.org>
 <8d46c46d-66d3-758b-b686-de371e0ba8ca@linaro.org>
 <Y419yaW1Q1uoXbtY@paquier.xyz>
 <20221205181449.47mhosidtmotemdl@awork3.anarazel.de>
 <6cfaaf3c-7fb5-51c8-92cf-ceac6303ef66@linaro.org>
 <Y5e/sWhwW+sMgVVh@paquier.xyz>
 <c1234f7e-e484-04a9-be2b-26db71f54af2@linaro.org>
 <20230117225131.jeptl4axjlbk5glz@awork3.anarazel.de>
From: Niyas Sait <niyas.sait@linaro.org>
In-Reply-To: <20230117225131.jeptl4axjlbk5glz@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/44f6c1b6-d712-b69b-82f2-8c525f59aeff%40linaro.org>
Precedence: bulk


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