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

[Russell Gallop via Phabricator via llvm-commits]

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:
> Quuxplusone wrote:
> > russell.gallop wrote:
> > > 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).
> > The version you're proposing here, using `THREAD_LOCAL_IF_THREADS_ENABLED`, specifically falls back to a single (per-process) global variable if `thread_local` isn't supported. So clearly the version with a single per-process variable does //work// (or else this patch is broken). You may be right that the version with a single per-process variable is slow, though.
> > 
> > It looks to me as if there is code in `llvm/include/llvm/Support/Compiler.h` to do exactly this macro-ization. @teemperor said don't use that code because it can fall back to `__thread` which doesn't work for non-PODs. However, your solution here is to use the `thread_local` keyword even on platforms where `__has_feature(cxx_thread_local)` is `false`, which seems wrong.
> > 
> > How about you roll the original patch back for now, just to get the build back in a stable state, and then either 
> > - submit a new patch that adds a new `LLVM_NONPOD_THREAD_LOCAL` macro to `Compiler.h` so that you can use it with `unique_ptr`, or
> > - rework this patch so that it doesn't rely on `~unique_ptr` to do its cleanup, so that then you can simply use `LLVM_THREAD_LOCAL`?
> > 
> > However, in either case, it seems like the `LLVM_THREAD_LOCAL` macro //can// fall back to nothing, i.e., a single per-process global variable. So whatever you do with `LLVM_THREAD_LOCAL`, you need to make sure that it's a pure optimization and that the underlying global-variable version still works correctly (if slowly).
> Btw, on Mac OSX, `LLVM_THREAD_LOCAL` expands to `__thread`. (I just tested it locally.)
Thanks for the info.

Multi-threaded time trace (D71059) reverted in 2e9bfa12ff. I'll take a look at this in the new year.


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

https://reviews.llvm.org/D71548