[Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature

[avt77]

Summary

To simplify the review and commit of D43578 I decided to spilt it in several small parts.

This patch is the first part of the new series of patches. This patch introduces a dedicated boolean to deal with -ftime-report Clang switch (instead of 'llvm::TimePassesIsEnabled' which was LLVM dependent and potentionally could increase Clang compilation time).

Next patch will show usage of llvm::NamedRegionTimer instead of llvm::TimeRegion - it will really improve the generating output of the given feature.

Then I'll show the new class to deal with reqursive time counters (instead of approach shown in CodeGenAction.cpp).

And then we'll try new time counters (Preprocessor, Include Files, Parsing, Sema, CodeGen, etc.) The final list of counters will grow in dpendence of possible future requirements.

Finally, I'm going to introduce the feature which is similar to this one.

Hope, all these efforts could be interesting for many of us.

[efriedma]

This makes sense.

include/clang/Frontend/Utils.h

241

Don't really like global variables, but I guess timers are global state anyway, so this isn't really making anything worse.

lib/Basic/FrontendTiming.cpp

18 ↗

(On Diff #142383)

Why is this in lib/Basic, when the declaration is in include/clang/Frontend/?

[avt77]

lib/Basic/FrontendTiming.cpp

18 ↗

(On Diff #142383)

Because this library is being linked to all others and as result this global variable could be seen in any Clang library w/o any changes in config files. Or you mean it should be moved from Frontend? But where? It's a frontend feature that's why I put its declaration there.

[efriedma]

lib/Basic/FrontendTiming.cpp

18 ↗

(On Diff #142383)

The include structure should match the library structure; if the definition needs to be in lib/Basic/, the declaration should be in include/clang/Basic.

[avt77]

I moved FrontendTiming.cpp from lib/Basic/ to lib/Frontend/.

[avt77]

Hi All,
Are there other issues related to this patch?
If NO could anyone give me LGTM? I need this patch committed to continue with recursive time counters.

[efriedma]

lib/Frontend/FrontendTiming.cpp

15

This should include clang/Frontend/Utils.h .

test/Frontend/ftime-report-template-decl.cpp

3

What is this test supposed to be testing? If you're just checking that we output the timers, this doesn't need to be so complicated.

We generally prefer to use %clang_cc1 for tests like this.

Please use -emit-llvm instead of -S if you don't actually need assembly.

[avt77]

test/Frontend/ftime-report-template-decl.cpp

3

You wrote "What is this test supposed to be testing? If you're just checking that we output the timers, this doesn't need to be so complicated."
I'm going to use this test to show new timers soon that's why it looks so complicated at the first glance. At the moment it simply checking the output of the existing counter but later it will show the new counters.

[avt77]

I fixed issues raised by efriedma.

[efriedma]

LGTM

[avt77]

It's terrible but my new test was failed again as result of commit of this patch!

/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Frontend/ftime-report-template-decl.cpp:155:11: error: expected string not found in input
CHECK: Code Generation Time
// ^

I don't understand how it's possible. The same problem raised when I committed D43578. Obviously, there is a situation when this test work w/o Code Generation and as result this test is fail because code generation time is zerro. Could anyone help me? How should I change the test? The simplest way is to remove this line but I don't like this idea.

[thakis]

In any case, when you see a test failing on bots and the fix isn't obvious,
revert first to get the bots back green.

[bjope]

In D45619#1075089, @avt77 wrote:

It's terrible but my new test was failed again as result of commit of this patch!

/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Frontend/ftime-report-template-decl.cpp:155:11: error: expected string not found in input
CHECK: Code Generation Time
// ^

I don't understand how it's possible. The same problem raised when I committed D43578. Obviously, there is a situation when this test work w/o Code Generation and as result this test is fail because code generation time is zerro. Could anyone help me? How should I change the test? The simplest way is to remove this line but I don't like this idea.

Not sure, but is perhaps the order in which timers are printed not 100% deterministic?
When I run the test "manually" without pipe to FileCheck I get:

===-------------------------------------------------------------------------===
                         Miscellaneous Ungrouped Timers
===-------------------------------------------------------------------------===

   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.0040 (100.0%)   0.0040 (100.0%)   0.0021 ( 70.7%)  Code Generation Time
   0.0000 (  0.0%)   0.0000 (  0.0%)   0.0009 ( 29.3%)  LLVM IR Generation Time
   0.0040 (100.0%)   0.0040 (100.0%)   0.0030 (100.0%)  Total

So when I run it without the pipe "Code Generation Time" is printed before "LLVM IR Generation Time".
However, the CHECK:s in the test case are in opposite order.
So I can't really understand why the test passes when I pipe it to FileCheck.

Anyway, if the order isn't deteministic, then a solution could be to use CHECK-DAG instead of CHECK for the checks that may be reordered. For example:

// CHECK: Miscellaneous Ungrouped Timers
// CHECK-DAG: Code Generation Time
// CHECK-DAG: LLVM IR Generation Time
// CHECK: Total
// CHECK: Clang front-end time report
// CHECK: Clang front-end timer
// CHECK: Total

Unless the timers are supposed to be printed in a certain order, then I guess you need to add a sort somewhere in the code.

[avt77]

In D45619#1075260, @thakis wrote:

In any case, when you see a test failing on bots and the fix isn't obvious,
revert first to get the bots back green.

I reverted it immediatly.

[avt77]

In D45619#1075385, @bjope wrote:

Anyway, if the order isn't deteministic, then a solution could be to use CHECK-DAG instead of CHECK for the checks that may be reordered. For example:

Thank you very much! I'll try to fix it in this way.

[bjope]

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So that is why it might be sorted differently on different buildbots (or when using pipe etc).

So I think a quick fix is to add -DAG to the checks that can be reorder and submit that fix.

[avt77]

In D45619#1075437, @bjope wrote:

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So that is why it might be sorted differently on different buildbots (or when using pipe etc).

So I think a quick fix is to add -DAG to the checks that can be reorder and submit that fix.

I don't see revert as well. But I did the following:

svn merge -c -330571 .

And everything was OK.

[bjope]

buildbots have been failing all day (and were still failing). So I pushed my suggested fix.
I hope that was OK.

[avt77]

In D45619#1075677, @bjope wrote:

buildbots have been failing all day (and were still failing). So I pushed my suggested fix.
I hope that was OK.

Thank you. It seems everything is OK now.

[RKSimon]

This was committed at rL330571

[craig.topper]

Correct me if I'm wrong, but after this change llvm no longer enables the timing of individual passes when -ftime-report is passed? Was that intentional?

[efriedma]

Oh, oops, I should have spotted that. No, the point of this was to separate the timing infrastructure, not to change the behavior of -ftime-report.

Should be a one-line change to add "llvm::TimePassesIsEnabled = TimePasses" back to BackendConsumer::BackendConsumer. (Maybe we can add flags to control them separately as a followup.)

[craig.topper]

Ok I'll add that back.

I'm unclear why the we would want to assign clang's FrontendTimesIsEnabled from inside CodeGenAction. If I'm understanding the intentions here, the goal was to add more timing infrastructure to clang. But if the enabling is tied to CodeGenAction, then doesn't that mean any new clang timers wouldn't work under -fsyntax-only?

[craig.topper]

Assignment restored in r339281. I'll file a bug to merge to 7.0

[avt77]

In D45619#1192803, @craig.topper wrote:

I'm unclear why the we would want to assign clang's FrontendTimesIsEnabled from inside CodeGenAction. If I'm understanding the intentions here, the goal was to add more timing infrastructure to clang. But if the enabling is tied to CodeGenAction, then doesn't that mean any new clang timers wouldn't work under -fsyntax-only?

Historically, it was done to get compilation times for separate files (to find the longest times). Because of that I did not assume usage of -fsyntax-only. Is it important? Should I move this activity into another place? If YES could you give me a hint where it should be done?

[efriedma]

Somewhere around CompilerInvocation::CreateFromArgs is probably appropriate.

include/clang/Frontend/Utils.h

 // When Clang->getFrontendOpts().DisableFree is set we don't delete some of the
 // global objects, but we don't want LeakDetectors to complain, so we bury them
 // in a globally visible array.
 void BuryPointer(const void *Ptr);
 template <typename T> void BuryPointer(std::unique_ptr<T> Ptr) {
   BuryPointer(Ptr.release());
 }
 
+// Frontend timing utils
+
+/// If the user specifies the -ftime-report argument on an Clang command line
+/// then the value of this boolean will be true, otherwise false.
+extern bool FrontendTimesIsEnabled;
+
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_UTILS_H

lib/CodeGen/BackendUtil.cpp

if (TM->addPassesToEmitFile(CodeGenPasses, OS, CGFT,
     return false;
   }
 
   return true;
 }
 
 void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
                                       std::unique_ptr<raw_pwrite_stream> OS) {
-  TimeRegion Region(llvm::TimePassesIsEnabled ? &CodeGenerationTime : nullptr);
+  TimeRegion Region(FrontendTimesIsEnabled ? &CodeGenerationTime : nullptr);
 
   setCommandLineOpts(CodeGenOpts);
 
   bool UsesCodeGen = (Action != Backend_EmitNothing &&
                       Action != Backend_EmitBC &&
                       Action != Backend_EmitLL);
   CreateTargetMachine(UsesCodeGen);
 

 /// Not all features are currently supported in this system, but where
 /// necessary it falls back to the legacy pass manager to at least provide
 /// basic functionality.
 ///
 /// This API is planned to have its functionality finished and then to replace
 /// `EmitAssembly` at some point in the future when the default switches.
 void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
     BackendAction Action, std::unique_ptr<raw_pwrite_stream> OS) {
-  TimeRegion Region(llvm::TimePassesIsEnabled ? &CodeGenerationTime : nullptr);
+  TimeRegion Region(FrontendTimesIsEnabled ? &CodeGenerationTime : nullptr);
   setCommandLineOpts(CodeGenOpts);
 
   // The new pass manager always makes a target machine available to passes
   // during construction.
   CreateTargetMachine(/*MustCreateTM*/ true);
   if (!TM)
     // This will already be diagnosed, just bail.
     return;

lib/CodeGen/CodeGenAction.cpp

BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
         : Diags(Diags), Action(Action), HeaderSearchOpts(HeaderSearchOpts),
           CodeGenOpts(CodeGenOpts), TargetOpts(TargetOpts), LangOpts(LangOpts),
           AsmOutStream(std::move(OS)), Context(nullptr),
           LLVMIRGeneration("irgen", "LLVM IR Generation Time"),
           LLVMIRGenerationRefCount(0),
           Gen(CreateLLVMCodeGen(Diags, InFile, HeaderSearchOpts, PPOpts,
                                 CodeGenOpts, C, CoverageInfo)),
           LinkModules(std::move(LinkModules)) {
-      llvm::TimePassesIsEnabled = TimePasses;
+      FrontendTimesIsEnabled = TimePasses;
     }
     llvm::Module *getModule() const { return Gen->GetModule(); }
     std::unique_ptr<llvm::Module> takeModule() {
       return std::unique_ptr<llvm::Module>(Gen->ReleaseModule());
     }
 
     CodeGenerator *getCodeGenerator() { return Gen.get(); }
 
     void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
       Gen->HandleCXXStaticMemberVarInstantiation(VD);
     }
 
     void Initialize(ASTContext &Ctx) override {
       assert(!Context && "initialized multiple times");
 
       Context = &Ctx;
 
-      if (llvm::TimePassesIsEnabled)
+      if (FrontendTimesIsEnabled)
         LLVMIRGeneration.startTimer();
 
       Gen->Initialize(Ctx);
 
-      if (llvm::TimePassesIsEnabled)
+      if (FrontendTimesIsEnabled)
         LLVMIRGeneration.stopTimer();
     }
 
     bool HandleTopLevelDecl(DeclGroupRef D) override {
       PrettyStackTraceDecl CrashInfo(*D.begin(), SourceLocation(),
                                      Context->getSourceManager(),
                                      "LLVM IR generation of declaration");
 
       // Recurse.
-      if (llvm::TimePassesIsEnabled) {
+      if (FrontendTimesIsEnabled) {
         LLVMIRGenerationRefCount += 1;
         if (LLVMIRGenerationRefCount == 1)
           LLVMIRGeneration.startTimer();
       }
 
       Gen->HandleTopLevelDecl(D);
 
-      if (llvm::TimePassesIsEnabled) {
+      if (FrontendTimesIsEnabled) {
         LLVMIRGenerationRefCount -= 1;
         if (LLVMIRGenerationRefCount == 0)
           LLVMIRGeneration.stopTimer();
       }
 
       return true;
     }
 
     void HandleInlineFunctionDefinition(FunctionDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                      Context->getSourceManager(),
                                      "LLVM IR generation of inline function");
-      if (llvm::TimePassesIsEnabled)
+      if (FrontendTimesIsEnabled)
         LLVMIRGeneration.startTimer();
 
       Gen->HandleInlineFunctionDefinition(D);
 
-      if (llvm::TimePassesIsEnabled)
+      if (FrontendTimesIsEnabled)
         LLVMIRGeneration.stopTimer();
     }
 
     void HandleInterestingDecl(DeclGroupRef D) override {
       // Ignore interesting decls from the AST reader after IRGen is finished.
       if (!IRGenFinished)
         HandleTopLevelDecl(D);
     }
bool LinkInModules() {
           return true;
       }
       return false; // success
     }
 
     void HandleTranslationUnit(ASTContext &C) override {
       {
         PrettyStackTraceString CrashInfo("Per-file LLVM IR generation");
-        if (llvm::TimePassesIsEnabled) {
+        if (FrontendTimesIsEnabled) {
           LLVMIRGenerationRefCount += 1;
           if (LLVMIRGenerationRefCount == 1)
             LLVMIRGeneration.startTimer();
         }
 
         Gen->HandleTranslationUnit(C);
 
-        if (llvm::TimePassesIsEnabled) {
+        if (FrontendTimesIsEnabled) {
           LLVMIRGenerationRefCount -= 1;
           if (LLVMIRGenerationRefCount == 0)
             LLVMIRGeneration.stopTimer();
         }
 
   IRGenFinished = true;
       }
 

lib/Frontend/CMakeLists.txt

add_clang_library(clangFrontend
   CompilerInvocation.cpp
   CreateInvocationFromCommandLine.cpp
   DependencyFile.cpp
   DependencyGraph.cpp
   DiagnosticRenderer.cpp
   FrontendAction.cpp
   FrontendActions.cpp
   FrontendOptions.cpp
+  FrontendTiming.cpp
   HeaderIncludeGen.cpp
   InitHeaderSearch.cpp
   InitPreprocessor.cpp
   LangStandards.cpp
   LayoutOverrideSource.cpp
   LogDiagnosticPrinter.cpp
   ModuleDependencyCollector.cpp
   MultiplexConsumer.cpp

lib/Frontend/FrontendTiming.cpp

+//===- FronendTiming.cpp - Implements Frontend timing utils  --------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file keps implementation of frontend timing utils.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Frontend/Utils.h"
+
+namespace clang {
+
+bool FrontendTimesIsEnabled = false;
+
+}

test/Frontend/ftime-report-template-decl.cpp

+// RUN: %clang_cc1 %s -emit-llvm -o - -ftime-report  2>&1 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -fdelayed-template-parsing -DDELAYED_TEMPLATE_PARSING -ftime-report  2>&1 | FileCheck %s
+
+// Template function declarations
+template <typename T>
+void foo();
+template <typename T, typename U>
+void foo();
+
+// Template function definitions.
+template <typename T>
+void foo() {}
+
+// Template class (forward) declarations
+template <typename T>
+struct A;
+template <typename T, typename U>
+struct b;
+template <typename>
+struct C;
+template <typename, typename>
+struct D;
+
+// Forward declarations with default parameters?
+template <typename T = int>
+class X1;
+template <typename = int>
+class X2;
+
+// Forward declarations w/template template parameters
+template <template <typename> class T>
+class TTP1;
+template <template <typename> class>
+class TTP2;
+template <template <typename X, typename Y> class T>
+class TTP5;
+
+// Forward declarations with non-type params
+template <int>
+class NTP0;
+template <int N>
+class NTP1;
+template <int N = 5>
+class NTP2;
+template <int = 10>
+class NTP3;
+template <unsigned int N = 12u>
+class NTP4;
+template <unsigned int = 12u>
+class NTP5;
+template <unsigned = 15u>
+class NTP6;
+template <typename T, T Obj>
+class NTP7;
+
+// Template class declarations
+template <typename T>
+struct A {};
+template <typename T, typename U>
+struct B {};
+
+namespace PR6184 {
+namespace N {
+template <typename T>
+void bar(typename T::x);
+}
+
+template <typename T>
+void N::bar(typename T::x) {}
+}
+
+// This PR occurred only in template parsing mode.
+namespace PR17637 {
+template <int>
+struct L {
+  template <typename T>
+  struct O {
+    template <typename U>
+    static void Fun(U);
+  };
+};
+
+template <int k>
+template <typename T>
+template <typename U>
+void L<k>::O<T>::Fun(U) {}
+
+void Instantiate() { L<0>::O<int>::Fun(0); }
+}
+
+namespace explicit_partial_specializations {
+typedef char (&oneT)[1];
+typedef char (&twoT)[2];
+typedef char (&threeT)[3];
+typedef char (&fourT)[4];
+typedef char (&fiveT)[5];
+typedef char (&sixT)[6];
+
+char one[1];
+char two[2];
+char three[3];
+char four[4];
+char five[5];
+char six[6];
+
+template <bool b>
+struct bool_ { typedef int type; };
+template <>
+struct bool_<false> {};
+
+#define XCAT(x, y) x##y
+#define CAT(x, y) XCAT(x, y)
+#define sassert(_b_) bool_<(_b_)>::type CAT(var, __LINE__);
+
+template <int>
+struct L {
+  template <typename T>
+  struct O {
+    template <typename U>
+    static oneT Fun(U);
+  };
+};
+template <int k>
+template <typename T>
+template <typename U>
+oneT L<k>::O<T>::Fun(U) { return one; }
+
+template <>
+template <>
+template <typename U>
+oneT L<0>::O<char>::Fun(U) { return one; }
+
+void Instantiate() {
+  sassert(sizeof(L<0>::O<int>::Fun(0)) == sizeof(one));
+  sassert(sizeof(L<0>::O<char>::Fun(0)) == sizeof(one));
+}
+}
+
+template <class>
+struct Foo {
+  template <class _Other>
+  using rebind_alloc = _Other;
+};
+template <class _Alloc>
+struct _Wrap_alloc {
+  template <class _Other>
+  using rebind_alloc = typename Foo<_Alloc>::template rebind_alloc<_Other>;
+  template <class>
+  using rebind = _Wrap_alloc;
+};
+_Wrap_alloc<int>::rebind<int> w;
+
+// CHECK: Miscellaneous Ungrouped Timers
+// CHECK: LLVM IR Generation Time
+// CHECK: Code Generation Time
+// CHECK: Total
+// CHECK: Clang front-end time report
+// CHECK: Clang front-end timer
+// CHECK: Total