Message-ID: <6cfaaf3c-7fb5-51c8-92cf-ceac6303ef66@linaro.org>
Date: Mon, 12 Dec 2022 13:38:37 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: [PATCH] Add native windows on arm64 support
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>
Cc: 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: <271611.1661998926@sss.pgh.pa.us> <YxA4F7EB3SBenvva@paquier.xyz>
 <CAB8KJ=jEZZa=2sgtpjq4raTaqh3vxx5GFFKWF=OZFHo6vp0cGg@mail.gmail.com>
 <CAFPTBD8CCT9BNdQQ0QELoo7HPqx26_Zkn0jxg9F9u+bPz-jnEg@mail.gmail.com>
 <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>
From: Niyas Sait <niyas.sait@linaro.org>
In-Reply-To: <20221205181449.47mhosidtmotemdl@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/6cfaaf3c-7fb5-51c8-92cf-ceac6303ef66%40linaro.org>
Precedence: bulk

Hi,

On 05/12/2022 18:14, Andres Freund wrote:

>> With meson gaining in maturity, perhaps that's not the most urgent
>> thing as we will likely remove src/tools/msvc/ soon but I'd rather do
>> that right anyway as much as I can to avoid an incorrect state in the
>> tree at any time in its history.
> 
> I'd actually argue that we should just not add win32 support to
> src/tools/msvc/.
> 
> 

I think the old build system specific part is really minimal in the 
patch. I can strip out those if that's preferred.


>> --- a/src/include/storage/s_lock.h
>> +++ b/src/include/storage/s_lock.h
>> @@ -708,13 +708,21 @@ typedef LONG slock_t;
>>  #define SPIN_DELAY() spin_delay()
>>  
>>  /* If using Visual C++ on Win64, inline assembly is unavailable.
>> - * Use a _mm_pause intrinsic instead of rep nop.
>> + * Use _mm_pause (x64) or __isb (arm64) intrinsic instead of rep nop.
>>   */
>>  #if defined(_WIN64)
>>  static __forceinline void
>>  spin_delay(void)
>>  {
>> +#ifdef _M_ARM64
>> +	/*
>> +	 * See spin_delay aarch64 inline assembly definition above for details
>> +	 * ref: https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics#BarrierRestrictions
>> +	*/
>> +	__isb(_ARM64_BARRIER_SY);
>> +#else
>>  	_mm_pause();
>> +#endif
>>  }
>>  #else
>>  static __forceinline void
> 
> This looks somewhat wrong to me. We end up with some ifdefs on the function
> defintion level, and some others inside the function body. I think it should
> be either or.
> 

Ok, I can add an MSVC/ARM64 specific function.

>> diff --git a/meson.build b/meson.build
>> index 725e10d815..e354ad7650 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1944,7 +1944,13 @@ int main(void)
>>   
>>   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
>> +  else
>> +
>> +    prog = '''
>>   #include <arm_acle.h>
>>   
>>   int main(void)
> Why does this need to be hardcoded? The compiler probe should just work for
> msvc.
> 

There are couple of minor issues in the code probe with MSVC such as 
arm_acle.h needs to be removed and requires an explicit import of 
intrin.h. But even with those fixes, USE_ARMV8_CRC32C would be set and 
no runtime CRC extension check will be performed. Since CRC extension is 
optional in ARMv8, It would be better to use the CRC variant with 
runtime check. So I end up following the x64 variant and hardcoded the 
flags in case of ARM64 and MSVC.


-- 
Niyas