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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 5 inline comments as done.
anton-afanasyev added inline comments.


================
Comment at: clang/lib/Parse/ParseAST.cpp:172
+  {
+    llvm::TimeTraceScope scope("Backend", "");
+    Consumer->HandleTranslationUnit(S.getASTContext());
----------------
rnk wrote:
> 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.
Yes, thanks.


================
Comment at: clang/lib/Sema/Sema.cpp:98
+        if (llvm::TimeTraceProfilerEnabled()) {
+          auto fe = SM.getFileEntryForID(SM.getFileID(Loc));
+          llvm::TimeTraceProfilerBegin(
----------------
rnk wrote:
> 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.
Ok


================
Comment at: llvm/lib/IR/LegacyPassManager.cpp:1632-1634
+  bool profileTime = llvm::TimeTraceProfilerEnabled();
+  if (profileTime)
+    llvm::TimeTraceProfilerBegin("OptFunction", F.getName().data());
----------------
rnk wrote:
> 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.
Ok, I'm to look for it.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:28
+
+static std::string EscapeString(const char *src) {
+  std::string os;
----------------
rnk wrote:
> 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.
Ok


================
Comment at: llvm/lib/Support/TimeProfiler.h:1
+//===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
+//
----------------
rnk wrote:
> I applied this patch locally to try it, and I noticed this header should be in llvm/include/llvm/Support, not llvm/lib/Support.
Oops, you're right, this is accidentally moved in patch! Thanks.


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

https://reviews.llvm.org/D58675