[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: clang/lib/Parse/ParseAST.cpp:172
+  {
+    llvm::TimeTraceScope scope("Backend", "");
+    Consumer->HandleTranslationUnit(S.getASTContext());
----------------
I think you may want to move this to `clang::EmitBackendOutput`, which is closer to real "backend-y" actions. I don't think there's any other heavy lifting that happens in HandleTranslationUnit, it looks like diagnostic teardown and setup for calling EmitBackendOutput.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3115-3118
+      TIME_TRACE_OR_NULL(
+          TagDecl != nullptr && isa<NamedDecl>(TagDecl)
+              ? cast<NamedDecl>(TagDecl)->getQualifiedNameAsString().data()
+              : "<anonymous>"));
----------------
For example, this would be simplified by a TimeTraceScope callable overload.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:237-238
+      "ParseTemplate",
+      TIME_TRACE_OR_NULL(DeclaratorInfo.getIdentifier() != nullptr
+                             ? DeclaratorInfo.getIdentifier()->getName().data()
+                             : "<unknown>"));
----------------
I guess this would be simplified as well with a callable.


================
Comment at: clang/lib/Sema/Sema.cpp:98
+        if (llvm::TimeTraceProfilerEnabled()) {
+          auto fe = SM.getFileEntryForID(SM.getFileID(Loc));
+          llvm::TimeTraceProfilerBegin(
----------------
This doesn't match the LLVM naming and auto usage conventions: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Yes, there is an active debate about changing the way we name variables, but please just match what we have for now.


================
Comment at: llvm/lib/IR/LegacyPassManager.cpp:1632-1634
+  bool profileTime = llvm::TimeTraceProfilerEnabled();
+  if (profileTime)
+    llvm::TimeTraceProfilerBegin("OptFunction", F.getName().data());
----------------
Someone is going to have to figure out where the equivalent annotations should live in the new passmanager. I wasn't able to find it just by looking myself, so I won't ask you to do it. I guess it's in llvm/include/llvm/IR/PassManager.h.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:28
+
+static std::string EscapeString(const char *src) {
+  std::string os;
----------------
Here and else where things should be named the LLVM way for consistency:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
`escapeString`, `Src`, etc. Tedious, I know.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:70
+
+  void Begin(const std::string &name, const std::string &detail) {
+    Entry e = {steady_clock::now(), {}, name, detail};
----------------
I'm tempted to micro-optimize this with StringRef and StringSaver, but I think it's unnecessary. Calling malloc may affect the profile, but probably not much.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:80
+
+    // only include sections longer than 500us
+    if (duration_cast<microseconds>(e.Duration).count() > 500)
----------------
Comments in this file need to be complete sentences with a leading capital and full stop.


================
Comment at: llvm/lib/Support/TimeProfiler.h:53
+struct TimeTraceScope {
+  TimeTraceScope(const char *name, const char *detail) {
+    if (TimeTraceProfilerInstance != nullptr)
----------------
It'd be nice if these took StringRefs, it would avoid a fair amount of `.data()` and `.c_str()`, but it messes up TIME_TRACE_OR_NULL. Another way to delay the work would be to have an `llvm::function_ref<StringRef()> detail` overload. The callable also allows the user to bind variables like this:
  TimeTraceScope timeScope("ParseTag", [&]() {
    if (auto *TD = dyn_cast_or_null<TagDecl>(D))
      return TD->getNameAsStr();
    return "";
  });
Instead of the `isa<TagDecl>(D) ? cast<TagDecl>(D)->getNameAsStr() : ""` pattern that I see, which repeats TagDecl.


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

https://reviews.llvm.org/D58675