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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:17
 struct TimeTraceProfiler;
-extern LLVM_THREAD_LOCAL TimeTraceProfiler *TimeTraceProfilerInstance;
+TimeTraceProfiler *GetTimeTraceProfilerInstance();
 
----------------
Use `camelCase()` to match style guide and other code in this header.


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


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:34
+// Per Thread instance
+LLVM_THREAD_LOCAL llvm::TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 } // namespace
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73639