Re: [PATCH] Add native windows on arm64 support

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