Re: [PATCH] Add native windows on arm64 support - Mailing list pgsql-hackers

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