public inbox for [email protected]  

help / color / mirror / Atom feed

From: Niyas Sait <[email protected]>
To: Andres Freund <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add native windows on arm64 support
Date: Mon, 12 Dec 2022 13:38:37 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<CAB8KJ=jEZZa=2sgtpjq4raTaqh3vxx5GFFKWF=OZFHo6vp0cGg@mail.gmail.com>
	<CAFPTBD8CCT9BNdQQ0QELoo7HPqx26_Zkn0jxg9F9u+bPz-jnEg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (90+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add native windows on arm64 support
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox