[builtins] Use Interlocked* intrinsics for atomics on MSVC

[mstorsjo]

Summary

Tested on MSVC 2013, 2015 and 2017 targeting X86, X64 and ARM.

This fixes building emutls.c for Windows for ARM (both with clang which don't need these atomics fallbacks at all, but just failed due to the immintrin.h include before, and with MSVC).

[marsupial]

I'm wondering if the #ifdef is now a dead code path entirely.
Do you have any insight into how ARM is getting __atomic_load_n and x86/64 not?

[mstorsjo]

In D36071#828513, @marsupial wrote:

I'm wondering if the #ifdef is now a dead code path entirely.
Do you have any insight into how ARM is getting __atomic_load_n and x86/64 not?

I had only tested it with clang for ARM, where the whole ifdef isn't used. I hadn't tested this with MSVC for ARM though (I only use compiler-rt when building with clang-mingw, not when building with MSVC), but it does indeed fail there. The _load/store_be_u32/64 intrinsics aren't perhaps the most canonical way of doing atomic loads/stores, but MSVC is pretty badly lacking in that department. They do document this though:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx

Simple reads and writes to properly-aligned 32-bit variables are atomic operations. In other words, you will not end up with only one portion of the variable updated; all bits are updated in an atomic fashion. However, access is not guaranteed to be synchronized. If two threads are reading and writing from the same variable, you cannot determine if one thread will perform its read operation before the other performs its write operation.

Simple reads and writes to properly aligned 64-bit variables are atomic on 64-bit Windows. Reads and writes to 64-bit values are not guaranteed to be atomic on 32-bit Windows. Reads and writes to variables of other sizes are not guaranteed to be atomic on any platform.

Although I'm not sure how well that applies to non-x86 platforms. I can update it to use the Interlocked* intrinsics that are available for many platforms. They're a bit hairy though since not all of them are available for all platforms, and it varies a little across MSVC versions as well.

[mstorsjo]

lib/builtins/emutls.c

219 ↗

(On Diff #109292)

X64 doesn't have Add64, but have got Or64_np. ARM (and according to MSVC 2017's intrin.h, ARM64 as well) have got Add64 though.

231 ↗

(On Diff #109292)

_InterlockedExchangePointer would ideally work on both 32 and 64 bit, but in MSVC 2013 for X86, this fails due to some kludge in MSVC's intrin.h.

[marsupial]

lib/builtins/emutls.c

219 ↗

(On Diff #109292)

The docs for these functions say it returns the original value.
Should be fine as 0 is added/ored, but a comment explaining the situation may be wise.

[mstorsjo]

lib/builtins/emutls.c

219 ↗

(On Diff #109292)

Sure, I can add that. Does my approach look sensible otherwise? Do you think I should add inline comments about the gotchas in different versions as motivation for each of them?

[marsupial]

lib/builtins/emutls.c

219 ↗

(On Diff #109292)

Overall seems proper/better. I did a quick check, and the previous load/store ops are significantly faster than the interlocked variants (which is possible why I chose them), but think it adds too much complexity and weirdness for expanding it to other architectures.

So just a simple comment about the or/add of zero is returning the original value that makes the operation equivalent to a pure load is enough for me.

I think the #ifdefs are enough/self-explanatory as to platform availability.

[mstorsjo]

Added the requested comment. Switched to the version of the intrinsics without a leading underscore, which made it work more consistently across MSVC versions, simplifying the code yet quite a bit.

[marsupial]

LGTM.

compiler-rt/trunk/lib/builtins/emutls.c

 }
 
 #else
 
 #include <windows.h>
 #include <malloc.h>
 #include <stdio.h>
 #include <assert.h>
-#include <immintrin.h>
 
 static LPCRITICAL_SECTION emutls_mutex;
 static DWORD emutls_tls_index = TLS_OUT_OF_INDEXES;
 
 typedef uintptr_t gcc_word;
 typedef void * gcc_pointer;
 
 static void win_error(DWORD last_err, const char *hint) {
if (value == NULL) {
             win_abort(err, "TlsGetValue");
     }
     return (emutls_address_array*) value;
 }
 
 /* Provide atomic load/store functions for emutls_get_index if built with MSVC.
  */
 #if !defined(__ATOMIC_RELEASE)
+#include <intrin.h>
 
 enum { __ATOMIC_ACQUIRE = 2, __ATOMIC_RELEASE = 3 };
 
 static __inline uintptr_t __atomic_load_n(void *ptr, unsigned type) {
     assert(type == __ATOMIC_ACQUIRE);
+    // These return the previous value - but since we do an OR with 0,
+    // it's equivalent to a plain load.
 #ifdef _WIN64
-    return (uintptr_t) _load_be_u64(ptr);
+    return InterlockedOr64(ptr, 0);
 #else
-    return (uintptr_t) _load_be_u32(ptr);
+    return InterlockedOr(ptr, 0);
 #endif
 }
 
 static __inline void __atomic_store_n(void *ptr, uintptr_t val, unsigned type) {
     assert(type == __ATOMIC_RELEASE);
-#ifdef _WIN64
-    _store_be_u64(ptr, val);
-#else
-    _store_be_u32(ptr, val);
-#endif
+    InterlockedExchangePointer((void *volatile *)ptr, (void *)val);
 }
 
 #endif
 
 #pragma warning (pop)
 
 #endif