[PATCH] D73639: [LLVM] Wrap extern TLS variable in getter Function

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:65
   TimeTraceScope(StringRef Name) {
-    if (TimeTraceProfilerInstance != nullptr)
+    if (GetTimeTraceProfilerInstance() != nullptr)
       timeTraceProfilerBegin(Name, StringRef(""));
----------------
mstorsjo wrote:
> rnk wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73639