Feedback-ID: id4a34324:Fastmail
Date: Mon, 5 Dec 2022 10:14:49 -0800
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael@paquier.xyz>
Cc: Niyas Sait <niyas.sait@linaro.org>,
	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>
Subject: Re: [PATCH] Add native windows on arm64 support
Message-ID: <20221205181449.47mhosidtmotemdl@awork3.anarazel.de>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y419yaW1Q1uoXbtY@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20221205181449.47mhosidtmotemdl%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-12-05 14:12:41 +0900, Michael Paquier 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/.


> --- 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.


> 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.


> @@ -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)
> +      # 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
> +    endif
>    endif
>  endif

And then this reindent wouldn't be needed.

Greetings,

Andres Freund