[PATCH] D43578: -ftime-report switch support in Clang

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin added a comment.

Hi,

I think it would be really instrumental to have a finer timing report, so thanks for your work! I tested the patch on CTMark with O0-g and didn't see detectable change either, so I think from compile-time impact this change is good. The patch needs some clean-up, but otherwise looks good to me.

Thanks,
Michael



================
Comment at: lib/CodeGen/CodeGenModule.cpp:59
 #include "llvm/Support/MD5.h"
+#include "llvm/Support/Timer.h"
 
----------------
No timers are added to this file, thus the header is redundant.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:64-65
 
+static const char *const GroupName = "codegenmodule";
+static const char *const GroupDescription = "===== Code Gen Module =====";
+
----------------
These are unused.


================
Comment at: lib/Frontend/FrontendAction.cpp:898-899
 
+static int action_number = 0;
+
 bool FrontendAction::Execute() {
----------------
Unused variable.


================
Comment at: lib/Lex/Pragma.cpp:32
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/Preprocessor.h"
----------------
Please don't reorder header files in the patch (you can commit such changes separately before the patch).


================
Comment at: lib/Lex/Pragma.cpp:44
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/Compiler.h"
----------------
Why do we need "Pass.h" here?


================
Comment at: lib/Lex/Pragma.cpp:89
                                             bool IgnoreNull) const {
+  llvm::NamedRegionTimer NRT("clangparser22", "PP Find Handler", GroupName,
+                             GroupDescription, llvm::TimePassesIsEnabled);
----------------
What does the number 22 mean (and other similar numbers in the names)?


================
Comment at: lib/Lex/Pragma.cpp:137
                                          PragmaIntroducerKind Introducer) {
+  llvm::NamedRegionTimer NRT("pppragma", "Handle Pragma Directive", GroupName,
+                             GroupDescription, llvm::TimePassesIsEnabled);
----------------
Typo in "pppragma"?


================
Comment at: lib/Parse/Parser.cpp:1468-1470
+  //  llvm::NamedRegionTimer NRT("templateannotate", "Template Annotation
+  //  operations", GroupName,
+  //                             GroupDescription, llvm::TimePassesIsEnabled);
----------------
Not-cleaned leftover of some sort?


================
Comment at: lib/Parse/Parser.cpp:1662
          "Cannot be a type or scope token!");
-
   if (Tok.is(tok::kw_typename)) {
----------------
Please don't include unnecessary whitespace changes (it applies to some other changes too).


https://reviews.llvm.org/D43578