[LLVM] Wrap extern TLS variable in getter Function

[zero9178]

Summary

This patch wraps an external thread local storage variable inside of a getter function and makes it have internal linkage. This allows LLVM to be built with BUILD_SHARED_LIBS on windows with MinGW. Additionally it allows Clang versions prior to 10 to compile current trunk.

[russell.gallop]

Additionally it allows Clang versions prior to 10 to compile current trunk.

I have been using Clang prior to 10 to compile without this change. Do you mean with BUILD_SHARED_LIBS for this as well?

[mstorsjo]

llvm/lib/Support/TimeProfiler.cpp

35

You can remove the llvm:: prefix, as this is within an anonymous namespace, like ThreadTimeTraceProfilerInstances.

[mstorsjo]

In D73639#1847333, @russell.gallop wrote:

Additionally it allows Clang versions prior to 10 to compile current trunk.

I have been using Clang prior to 10 to compile without this change. Do you mean with BUILD_SHARED_LIBS for this as well?

He meant when targeting mingw. There were issues with code generation for mingw targets, when accessing one TLS variable from multiple object files but that's fixed in the 10.0 branch and master.

But one can't access a TLS variable directly from a different DLL (as before with extern LLVM_THREAD_LOCAL) at all, which would be necessary when building with BUILD_SHARED_LIBS.

[zero9178]

In D73639#1847333, @russell.gallop wrote:

Additionally it allows Clang versions prior to 10 to compile current trunk.

I have been using Clang prior to 10 to compile without this change. Do you mean with BUILD_SHARED_LIBS for this as well?

I believe this only occurs when using clang with MinGW as it needs to generate extra wrapper symbols that previously didn't have some needed comdat attributes

llvm/lib/Support/TimeProfiler.cpp

35

I don't fully understand. TimeTraceProfiler is in the namespace llvm so I'd get an unknown typename if I were to remove it

[rnk]

llvm/include/llvm/Support/TimeProfiler.h

17

Use camelCase() to match style guide and other code in this header.

65

This puts a function call on the TimeTrace fast path (tracing disabled). Can you please see if this is at all measurable? Try doing three compilations of something big in clang, like SemaDeclAttr.cpp or X86ISelLowering.cpp, with and without this change. I would suggest doing it on Linux, where TLS access is optimized, so any difference would be more visible.

llvm/lib/Support/TimeProfiler.cpp

35

Typically LLVM uses using namespace llvm; in cpp files, but I see that this file lacks it. Perhaps it should be added.

[mstorsjo]

llvm/include/llvm/Support/TimeProfiler.h

65

Worst case, one could have #ifdef _WIN32 #define TimeTraceProfilerInstance getTimeTraceProfilerInstance() #else extern LLVM_THREAD_LOCAL TimeTraceProfilerInstance; #endif, but if it's negligible, it's of course simpler without that.

llvm/lib/Support/TimeProfiler.cpp

35

Oh, sorry, I misread - I thought it was on the variable name - as type qualifier it's ok.

[mstorsjo]

llvm/include/llvm/Support/TimeProfiler.h

65

I tried benchmarking it, and it's hard to say if there's any noticeable difference.

On a clang built with GCC 7.3, building X86ISelLowering.cpp with this clang from current master takes 47.467s, 47.186s, 47.488s in three attempts, while it takes 47.675s, 47.287s, 47.336s with this patch. After a few more tests with both, the lowest measurement for the unpatched version is lower than the lowest for the patched version, but there's lots of overlap between the range for measurements for the two versions.

If clang is built with LTO, there should't really be any difference at all, as far as I understand.

[zero9178]

I also just measured compiling X86ISelLowering. Once on trunk and once with the patch applied. Using GCC 9.2 as the compiler got me 31.572, 31.843 and 31.936 for the current master and 32.241, 32.370 and 32.306 with the patch applied. So it's measurable on my machine

[zero9178]

Changed function name to camelCase, Added using namespace llvm

[mstorsjo]

In D73639#1848054, @zero9178 wrote:

I also just measured compiling X86ISelLowering. Once on trunk and once with the patch applied. Using GCC 9.2 as the compiler got me 31.572, 31.843 and 31.936 for the current master and 32.241, 32.370 and 32.306 with the patch applied. So it's measurable on my machine

I realized I had built my clang with asserts enabled, which might dilute the effect of the change. But despite disabling asserts, I can't measure any significant difference due to this change - it's almost the contrary effect, this change actually seems to make it marginally faster (both when built with GCC 7 and Clang 9).

[zero9178]

In D73639#1848892, @mstorsjo wrote:

In D73639#1848054, @zero9178 wrote:

I also just measured compiling X86ISelLowering. Once on trunk and once with the patch applied. Using GCC 9.2 as the compiler got me 31.572, 31.843 and 31.936 for the current master and 32.241, 32.370 and 32.306 with the patch applied. So it's measurable on my machine

I realized I had built my clang with asserts enabled, which might dilute the effect of the change. But despite disabling asserts, I can't measure any significant difference due to this change - it's almost the contrary effect, this change actually seems to make it marginally faster (both when built with GCC 7 and Clang 9).

My Machine (a Ryzen 1950X) is probably a rather exotic machine compared to others so I think it's safe to assume that your results are more representative of the real world

[rnk]

Honestly, the numbers from both runs seem within the noise. I'm not very good at stats, though. >_>

So, I think this is good to go.

[zero9178]

Thank you! Would need someone to commit this for me

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/Support/raw_ostream.h"
 
 namespace llvm {
 
 struct TimeTraceProfiler;
-extern LLVM_THREAD_LOCAL TimeTraceProfiler *TimeTraceProfilerInstance;
+TimeTraceProfiler *getTimeTraceProfilerInstance();
 
 /// Initialize the time trace profiler.
 /// This sets up the global \p TimeTraceProfilerInstance
 /// variable to be the profiler instance.
 void timeTraceProfilerInitialize(unsigned TimeTraceGranularity,
                                  StringRef ProcName);
 
 /// Cleanup the time trace profiler, if it was initialized.
 void timeTraceProfilerCleanup();
 
 /// Finish a time trace profiler running on a worker thread.
 void timeTraceProfilerFinishThread();
 
 /// Is the time trace profiler enabled, i.e. initialized?
 inline bool timeTraceProfilerEnabled() {
-  return TimeTraceProfilerInstance != nullptr;
+  return getTimeTraceProfilerInstance() != nullptr;
 }
 
 /// Write profiling data to output file.
 /// Data produced is JSON, in Chrome "Trace Event" format, see
 /// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
 void timeTraceProfilerWrite(raw_pwrite_stream &OS);
 
 /// Manually begin a time section, with the given \p Name and \p Detail.
struct TimeTraceScope {
 
   TimeTraceScope() = delete;
   TimeTraceScope(const TimeTraceScope &) = delete;
   TimeTraceScope &operator=(const TimeTraceScope &) = delete;
   TimeTraceScope(TimeTraceScope &&) = delete;
   TimeTraceScope &operator=(TimeTraceScope &&) = delete;
 
   TimeTraceScope(StringRef Name) {
-    if (TimeTraceProfilerInstance != nullptr)
+    if (getTimeTraceProfilerInstance() != nullptr)
       timeTraceProfilerBegin(Name, StringRef(""));
   }
   TimeTraceScope(StringRef Name, StringRef Detail) {
-    if (TimeTraceProfilerInstance != nullptr)
+    if (getTimeTraceProfilerInstance() != nullptr)
       timeTraceProfilerBegin(Name, Detail);
   }
   TimeTraceScope(StringRef Name, llvm::function_ref<std::string()> Detail) {
-    if (TimeTraceProfilerInstance != nullptr)
+    if (getTimeTraceProfilerInstance() != nullptr)
       timeTraceProfilerBegin(Name, Detail);
   }
   ~TimeTraceScope() {
-    if (TimeTraceProfilerInstance != nullptr)
+    if (getTimeTraceProfilerInstance() != nullptr)
       timeTraceProfilerEnd();
   }
 };
 
 } // end namespace llvm
 
 #endif

llvm/lib/Support/TimeProfiler.cpp

 #include <algorithm>
 #include <cassert>
 #include <chrono>
 #include <mutex>
 #include <string>
 #include <vector>
 
 using namespace std::chrono;
+using namespace llvm;
 
 namespace {
 std::mutex Mu;
-std::vector<llvm::TimeTraceProfiler *>
+// List of all instances
+std::vector<TimeTraceProfiler *>
     ThreadTimeTraceProfilerInstances; // guarded by Mu
+// Per Thread instance
+LLVM_THREAD_LOCAL TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 } // namespace
 
 namespace llvm {
 
-LLVM_THREAD_LOCAL TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
+TimeTraceProfiler *getTimeTraceProfilerInstance() {
+  return TimeTraceProfilerInstance;
+}
 
 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;
 
 struct Entry {