[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(""));
----------------
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.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:34
+// Per Thread instance
+LLVM_THREAD_LOCAL llvm::TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 } // namespace
----------------
rnk wrote:
> zero9178 wrote:
> > mstorsjo wrote:
> > > You can remove the `llvm::` prefix, as this is within an anonymous namespace, like ThreadTimeTraceProfilerInstances.
> > I don't fully understand. TimeTraceProfiler is in the namespace llvm so I'd get an unknown typename if I were to remove it
> Typically LLVM uses `using namespace llvm;` in cpp files, but I see that this file lacks it. Perhaps it should be added.
Oh, sorry, I misread - I thought it was on the variable name - as type qualifier it's ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73639