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