[PATCH] D71548: Fix time trace multi threaded support with LLVM_ENABLE_THREADS=OFF

Mon Dec 23 06:30:34 PST 2019

russell.gallop marked an inline comment as done.
russell.gallop added inline comments.

================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:19
+#define THREAD_LOCAL_IF_THREADS_ENABLED
+#endif
+
----------------
Quuxplusone wrote:
> This patch still doesn't unbreak the build on Mac OSX:
> 
> ```
> /llvm-project/llvm/include/llvm/Support/TimeProfiler.h:24:8: error: thread-local storage is not supported for the current target
> extern THREAD_LOCAL_IF_THREADS_ENABLED std::unique_ptr<TimeTraceProfiler>
>        ^
> ```
> 
> Personally, I'd like to see the original patch reverted until you can find a way to do it without `thread_local`. ...And it seems like you //do// have a way? If the code works without using `thread_local` at all, then what's the logic behind adding `thread_local` to it conditionally? Given that the version without `thread_local` works fine, can we just use that version on all platforms?
Ah, I thought that the Mac OSX build was using LLVM_ENABLE_THREADS=OFF, but it has that on, but doesn't have C++11 thread_local. Does it use the gcc __thread (e.g. for LLVM_THREAD_LOCAL)?

> And it seems like you do have a way?

I'm not sure what you're referring to here:

a) The original RFC for LLD time trace (https://reviews.llvm.org/D69043) had a lock heavy version which was massively inefficient.
b) This works without thread_local when LLVM_ENABLE_THREADS is OFF because there is only one thread. That doesn't achieve the desired effect when LLVM_ENABLE_THREADS is ON.
c) Using LLVM_THREAD_LOCAL again, and getting that working with __thread. I think this could be made to work.

> Given that the version without thread_local works fine, can we just use that version on all platforms?

b) above doesn't work multi-threaded. a) above is very inefficient.

Apologies if I've misunderstood.

So I believe that you're suggesting is a version which is able to use another form of TLS rather than c++11 thread_local? If so, I'll try to get a version going which works like that (and revert in the meantime).

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71548/new/

https://reviews.llvm.org/D71548