Fix time trace multi threaded support with LLVM_ENABLE_THREADS=OFF

[russell.gallop]

Summary

Following on from https://reviews.llvm.org/rG8ddcd1dc26bad9d97bbf34cf4fc91dbf3c153431.

As pointed out here: https://reviews.llvm.org/D71059 this does not build on some systems with LLVM_ENABLE_THREADS=OFF.

[Kai]

This comment was removed by Kai.

[Kai]

LGTM. Thanks!

[ruiu]

LGTM

[teemperor]

This broke a whole lot of bots like http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/28924 or http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/30851:

[2/3225] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o
FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o 
/usr/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Support -I/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/llvm/lib/Support -Iinclude -I/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3    -UNDEBUG -std=c++14  -fno-exceptions -fno-rtti -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/TimeProfiler.cpp.o -c /home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/llvm/lib/Support/TimeProfiler.cpp
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/llvm/lib/Support/TimeProfiler.cpp:36:54: error: non-local variable 'llvm::TimeTraceProfilerInstance' declared '__thread' has a non-trivial destructor
 LLVM_THREAD_LOCAL std::unique_ptr<TimeTraceProfiler> TimeTraceProfilerInstance;
                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/llvm/lib/Support/TimeProfiler.cpp:36:54: note: C++11 'thread_local' allows dynamic initialization and destruction

[teemperor]

I don't think LLVM_THREAD_LOCAL is correct here. unique_ptr isn't a POD so this doesn't really fit to the use case of` LLVM_THREAD_LOCAL`. Maybe we should have a LLVM_NONTRIVIAL_THREAD_LOCAL. that doesn't fall back to __thread. Or we just use raw pointer here instead of a global unique_ptr (it seems that we anyway use it here just as an annotation to describe memory ownership?).

[russell.gallop]

In D71548#1787415, @teemperor wrote:

I don't think LLVM_THREAD_LOCAL is correct here. unique_ptr isn't a POD so this doesn't really fit to the use case of` LLVM_THREAD_LOCAL`. Maybe we should have a LLVM_NONTRIVIAL_THREAD_LOCAL. that doesn't fall back to __thread. Or we just use raw pointer here instead of a global unique_ptr (it seems that we anyway use it here just as an annotation to describe memory ownership?).

I'd prefer a "thread_local if threads enabled" as the unique_ptr helps enforce ownership when https://reviews.llvm.org/D71060 lands.

Interestingly, thread_local was okay on all public bots, the only problem was with LLVM_ENABLE_THREADS=OFF so maybe LLVM_THREAD_LOCAL could be simplified to "thread_local if threads enabled" now (removing declspec(thread) and thread). I don't know enough about how that is used to know whether that would be okay.

[russell.gallop]

More specific fix. Just avoid thread_local when LLVM_ENABLE_THREADS=OFF.

[russell.gallop]

@ruiu or @Kai I would like this to go in with D71060. Are you okay with this revised fix?

Thanks

[russell.gallop]

Ping! Okay to land this? (I moved it back to needs review as this was very different from before).

[• Quuxplusone]

llvm/include/llvm/Support/TimeProfiler.h

19

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?

[russell.gallop]

llvm/include/llvm/Support/TimeProfiler.h

19

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

[• Quuxplusone]

llvm/include/llvm/Support/TimeProfiler.h

19

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

[• Quuxplusone]

llvm/include/llvm/Support/TimeProfiler.h

19

Btw, on Mac OSX, LLVM_THREAD_LOCAL expands to __thread. (I just tested it locally.)

[russell.gallop]

llvm/include/llvm/Support/TimeProfiler.h

19

Thanks for the info.

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

[russell.gallop]

Abandoning this review. Will roll fix for these issues into new version on D71059.

llvm/include/llvm/Support/TimeProfiler.h

 //===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TIME_PROFILER_H
 #define LLVM_SUPPORT_TIME_PROFILER_H
 
+#include "llvm/Config/llvm-config.h"
 #include "llvm/Support/raw_ostream.h"
 
+#if LLVM_ENABLE_THREADS
+#define THREAD_LOCAL_IF_THREADS_ENABLED thread_local
+#else
+#define THREAD_LOCAL_IF_THREADS_ENABLED
+#endif
+
 namespace llvm {
 
 struct TimeTraceProfiler;
-extern thread_local std::unique_ptr<TimeTraceProfiler>
+extern THREAD_LOCAL_IF_THREADS_ENABLED std::unique_ptr<TimeTraceProfiler>
     TimeTraceProfilerInstance;
 
 /// Initialize the time trace profiler.
 /// This sets up the global \p TimeTraceProfilerInstance
 /// variable to be the profiler instance.
 void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
                                  StringRef ProcName);
 

llvm/lib/Support/TimeProfiler.cpp

 namespace {
 std::mutex Mu;
 std::vector<std::unique_ptr<llvm::TimeTraceProfiler>>
     ThreadTimeTraceProfilerInstances; // guarded by Mu
 } // namespace
 
 namespace llvm {
 
-thread_local std::unique_ptr<TimeTraceProfiler> TimeTraceProfilerInstance =
+THREAD_LOCAL_IF_THREADS_ENABLED std::unique_ptr<TimeTraceProfiler> TimeTraceProfilerInstance =
     nullptr;
 
 typedef duration<steady_clock::rep, steady_clock::period> DurationType;
 typedef time_point<steady_clock> TimePointType;
 typedef std::pair<size_t, DurationType> CountAndDurationType;
 typedef std::pair<std::string, CountAndDurationType>
     NameAndCountAndDurationType;