Page MenuHomePhabricator

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

Authored by russell.gallop on Jan 8 2020, 6:46 AM.

Details

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

Diff Detail

Event Timeline

russell.gallop created this revision.Jan 8 2020, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2020, 6:46 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
dblaikie accepted this revision.Jan 8 2020, 8:00 AM
dblaikie added a subscriber: rnk.

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

This revision is now accepted and ready to land.Jan 8 2020, 8:00 AM

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 added inline comments.
llvm/include/llvm/Support/Compiler.h
522

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).

Fix condition

russell.gallop marked 2 inline comments as done.Jan 10 2020, 5:25 AM
russell.gallop added inline comments.
llvm/include/llvm/Support/Compiler.h
522

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

rnk accepted this revision.Jan 14 2020, 3:47 PM

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 marked an inline comment as done.Jan 15 2020, 3:07 AM
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).

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Jan 15 2020, 2:19 PM

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.

In D72399#1822902, @rnk 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.