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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits