[PATCH] D136022: [clang] Add time profile for constant evaluation

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D136022#3861245 <https://reviews.llvm.org/D136022#3861245>, @jloser wrote:

> I'd like to see some tests through before I approve.

Thanks for the greenlight!

I added a test that compiles a chunk of code and then checks the time trace graph in a human-readable form.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1740-1746
+  llvm::TimeTraceScope TimeScope("CheckConstexprFunctionDefinition", [&] {
+    std::string Name;
+    llvm::raw_string_ostream OS(Name);
+    NewFD->getNameForDiagnostic(OS, Context.getPrintingPolicy(),
+                                /*Qualified=*/true);
+    return Name;
+  });
----------------
aaron.ballman wrote:
> Huh, I'm a bit surprised that the checking in this function isn't dominated by the time spent in `Expr::isPotentialConstantExpr()` -- if you add the time tracing to all of the constant expression evaluation functions, do you still need a time trace here in Sema?
Thanks! Removed trace here, added to `Expr::isPotentialConstantExpr()`. This method is called inside of `CheckConstexprFunctionDefinition` and really takes almost all the time.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17544
+  {
+    llvm::TimeTraceScope TimeScope("EvaluateAsConstantExpr", [&] {
+      return CE->getSourceRange().printToString(SemaRef.getSourceManager());
----------------
aaron.ballman wrote:
> Why not push this down into `EvaluateAsConstantExpr()`? (Then the name you give the scope will also be more accurate.)
Thanks! I wrote on this level because I didn't manage to get `SemaRef.getSourceManager()` without `SemaRef` (we pass down `SemaRef.getASTContext()`).

Anyway I found out that we can use `SemaRef.getASTContext().getSourceManager()` =)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136022