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


Reply via email to