[Support] Replace Windows __declspec(thread) with thread_local in LLVM_THREAD_LOCAL

[russell.gallop]

Summary

Windows minimum host tools version is now VS2017 [1], which supports C++11 thread_local so use this for LLVM_THREAD_LOCAL instead of declspec(thread). According to [2], thread_local is implemented with declspec(thread) so this should be NFC.

I believe the only host platform that doesn't support C++ thread_local is Apple Clang 6.0.

[1] https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library
[2] https://docs.microsoft.com/en-us/cpp/cpp/thread?view=vs-2017

[dblaikie]

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

[russell.gallop]

In D72399#1810145, @dblaikie wrote:

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

Thanks. @rnk, do you have any objections?

[mstorsjo]

llvm/include/llvm/Support/Compiler.h

516

I'd suggest using || instead of or here, for consistency, even if both probably(?) are supported (I wasn't aware of this existing at all before).

[russell.gallop]

Fix condition

[russell.gallop]

llvm/include/llvm/Support/Compiler.h

516

Sorry, I'd fixed that locally but uploaded an old patch. Thanks for spotting this. Fixed now.

[rnk]

lgtm

I checked that __declspec(thread) and thread_local seem to be functionally equivalent with MSVC.

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

[russell.gallop]

In D72399#1820866, @rnk wrote:

lgtm

I checked that __declspec(thread) and thread_local seem to be functionally equivalent with MSVC.

Thanks.

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

I want something (for D71059) which will use C++ thread_local if LLVM_ENABLE_THREADS is enabled but doesn't need to enforce trivially constructible/destructible as long as host tools support is there. If this macro is specifically designed for more limited uses, maybe I need something slightly different that prefers thread_local (over __thread).

[rnk]

In D72399#1821475, @russell.gallop wrote:

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

Yes, LLVM still supports platforms which do not support C++11 thread_local, so for LLVM, there is still a requirement that things be trivially constructible/destructible. If we used __thread when available, the compiler would enforce that check earlier, rather than letting the contributor commit, push, and find out about this requirement from a buildbot.

[dexonsmith]

In D72399#1822902, @rnk wrote:

In D72399#1821475, @russell.gallop wrote:

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

Yes, LLVM still supports platforms which do not support C++11 thread_local, so for LLVM, there is still a requirement that things be trivially constructible/destructible. If we used __thread when available, the compiler would enforce that check earlier, rather than letting the contributor commit, push, and find out about this requirement from a buildbot.

IIRC, __thread is more efficient than thread_local when it's valid, since thread_local uses dynamic synchronization even when there are no dynamic initializers. We should prefer __thread when it's available.

llvm/include/llvm/Support/Compiler.h

 #define LLVM_PRETTY_FUNCTION __func__
 #endif
 
 /// \macro LLVM_THREAD_LOCAL
 /// A thread-local storage specifier which can be used with globals,
 /// extern globals, and static globals.
 ///
 /// This is essentially an extremely restricted analog to C++11's thread_local
-/// support, and uses that when available. However, it falls back on
-/// platform-specific or vendor-provided extensions when necessary. These
-/// extensions don't support many of the C++11 thread_local's features. You
-/// should only use this for PODs that you can statically initialize to
-/// some constant value. In almost all circumstances this is most appropriate
-/// for use with a pointer, integer, or small aggregation of pointers and
-/// integers.
+/// support. It uses thread_local if available, falling back on gcc __thread
+/// if not. __thread doesn't support many of the C++11 thread_local's
+/// features. You should only use this for PODs that you can statically
+/// initialize to some constant value. In almost all circumstances this is most
+/// appropriate for use with a pointer, integer, or small aggregation of
+/// pointers and integers.
 #if LLVM_ENABLE_THREADS
-#if __has_feature(cxx_thread_local)
+#if __has_feature(cxx_thread_local) || defined(_MSC_VER)
 #define LLVM_THREAD_LOCAL thread_local
-#elif defined(_MSC_VER)
-// MSVC supports this with a __declspec.
-#define LLVM_THREAD_LOCAL __declspec(thread)
 #else
 // Clang, GCC, and other compatible compilers used __thread prior to C++11 and
 // we only need the restricted functionality that provides.
 #define LLVM_THREAD_LOCAL __thread
 #endif
 #else // !LLVM_ENABLE_THREADS
 // If threading is disabled entirely, this compiles to nothing and you get
 // a normal global variable.