subject:"\[PATCH\] D58675\: \[clang\] Adds `\-ftime\-trace` option to clang that produces Chrome `chrome\:\/\/tracing` compatible JSON profiling output dumps"

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-27 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked an inline comment as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > anton-afanasyev wrote:
> > > lebedev.ri wrote:
> > > > anton-afanasyev wrote:
> > > > > lebedev.ri wrote:
> > > > > > anton-afanasyev wrote:
> > > > > > > anton-afanasyev wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > This isn't covered by any timer; if you look in 
> > > > > > > > > `BackendUtil.cpp`,
> > > > > > > > > `EmitAssemblyHelper` actually has 
> > > > > > > > > `CodeGenerationTime("codegen", "Code Generation Time")` timer.
> > > > > > > > Thanks, I'm to add it.
> > > > > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > > > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > > > > timings, i think? :)
> > > > > > 
> > > > > > I have noticed this because when i looked at the produced time 
> > > > > > flame graph,
> > > > > > there's large section in the end that is covered only by 
> > > > > > `"Backend"` timer,
> > > > > > but nothing else. Now, i'm not going to say whether or not that 
> > > > > > extra section
> > > > > > should or should not be within `"Backend"` timer, but it certainly 
> > > > > > should *also*
> > > > > > be within `"codegen"` timer. Or is there no codegen time spent 
> > > > > > there?
> > > > > > {F10062322}
> > > > > > {F10062316}
> > > > > "Mostly coincides" here means "identical" I believe, the difference 
> > > > > is auxiliary stuff.
> > > > > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is 
> > > > > outer for `"codegen"` one.
> > > > Then we are talking about different things using the same name.
> > > > There are two distinct codegen steps:
> > > > 1. clang AST -> LLVM IR codegen
> > > > (after that, all the opt passes run)
> > > > 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> > > > passes.
> > > > 
> > > > Those are *different* codegen's.
> > > Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named 
> > > just "Backend".
> > Aha. So this is what i //expected// to see, apparently: 
> > {F10063128} {F10063119}
> > ```
> > diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
> > b/clang/lib/CodeGen/BackendUtil.cpp
> > index 71ae8fd4fb0..d89d12612f8 100644
> > --- a/clang/lib/CodeGen/BackendUtil.cpp
> > +++ b/clang/lib/CodeGen/BackendUtil.cpp
> > @@ -910,6 +910,7 @@ void EmitAssemblyHelper::EmitAssembly(BackendAction 
> > Action,
> >  
> >{
> >  PrettyStackTraceString CrashInfo("Code generation");
> > +llvm::TimeTraceScope TimeScope("BackendCodegen", StringRef(""));
> >  CodeGenPasses.run(*TheModule);
> >}
> > ```
> > 
> > But that actually brings me to another question - what about 
> > `PrettyStackTraceString CrashInfo("Per-function optimization");`?
> > Should that be wrapped into some section, too?
> > I'm less certain here.
> Ok, you've talked about `Timer CodeGenerationTime`, which corresponds to 
> `Backend` scope.
> As for this `BackendCodegen`, adding this I would prefer to add adjacent 
> `"Per-function optimization"` and `"Per-module optimization passes"` together.
Changes are here: https://reviews.llvm.org/D68161


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > anton-afanasyev wrote:
> > > > lebedev.ri wrote:
> > > > > anton-afanasyev wrote:
> > > > > > anton-afanasyev wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > This isn't covered by any timer; if you look in 
> > > > > > > > `BackendUtil.cpp`,
> > > > > > > > `EmitAssemblyHelper` actually has 
> > > > > > > > `CodeGenerationTime("codegen", "Code Generation Time")` timer.
> > > > > > > Thanks, I'm to add it.
> > > > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > > > timings, i think? :)
> > > > > 
> > > > > I have noticed this because when i looked at the produced time flame 
> > > > > graph,
> > > > > there's large section in the end that is covered only by `"Backend"` 
> > > > > timer,
> > > > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > > > section
> > > > > should or should not be within `"Backend"` timer, but it certainly 
> > > > > should *also*
> > > > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > > > {F10062322}
> > > > > {F10062316}
> > > > "Mostly coincides" here means "identical" I believe, the difference is 
> > > > auxiliary stuff.
> > > > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer 
> > > > for `"codegen"` one.
> > > Then we are talking about different things using the same name.
> > > There are two distinct codegen steps:
> > > 1. clang AST -> LLVM IR codegen
> > > (after that, all the opt passes run)
> > > 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> > > passes.
> > > 
> > > Those are *different* codegen's.
> > Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
> > "Backend".
> Aha. So this is what i //expected// to see, apparently: 
> {F10063128} {F10063119}
> ```
> diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
> b/clang/lib/CodeGen/BackendUtil.cpp
> index 71ae8fd4fb0..d89d12612f8 100644
> --- a/clang/lib/CodeGen/BackendUtil.cpp
> +++ b/clang/lib/CodeGen/BackendUtil.cpp
> @@ -910,6 +910,7 @@ void EmitAssemblyHelper::EmitAssembly(BackendAction 
> Action,
>  
>{
>  PrettyStackTraceString CrashInfo("Code generation");
> +llvm::TimeTraceScope TimeScope("BackendCodegen", StringRef(""));
>  CodeGenPasses.run(*TheModule);
>}
> ```
> 
> But that actually brings me to another question - what about 
> `PrettyStackTraceString CrashInfo("Per-function optimization");`?
> Should that be wrapped into some section, too?
> I'm less certain here.
Ok, you've talked about `Timer CodeGenerationTime`, which corresponds to 
`Backend` scope.
As for this `BackendCodegen`, adding this I would prefer to add adjacent 
`"Per-function optimization"` and `"Per-module optimization passes"` together.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > anton-afanasyev wrote:
> > > lebedev.ri wrote:
> > > > anton-afanasyev wrote:
> > > > > anton-afanasyev wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > > > "Code Generation Time")` timer.
> > > > > > Thanks, I'm to add it.
> > > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > > timings, i think? :)
> > > > 
> > > > I have noticed this because when i looked at the produced time flame 
> > > > graph,
> > > > there's large section in the end that is covered only by `"Backend"` 
> > > > timer,
> > > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > > section
> > > > should or should not be within `"Backend"` timer, but it certainly 
> > > > should *also*
> > > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > > {F10062322}
> > > > {F10062316}
> > > "Mostly coincides" here means "identical" I believe, the difference is 
> > > auxiliary stuff.
> > > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer 
> > > for `"codegen"` one.
> > Then we are talking about different things using the same name.
> > There are two distinct codegen steps:
> > 1. clang AST -> LLVM IR codegen
> > (after that, all the opt passes run)
> > 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> > passes.
> > 
> > Those are *different* codegen's.
> Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
> "Backend".
Aha. So this is what i //expected// to see, apparently: 
{F10063128} {F10063119}
```
diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index 71ae8fd4fb0..d89d12612f8 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -910,6 +910,7 @@ void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
 
   {
 PrettyStackTraceString CrashInfo("Code generation");
+llvm::TimeTraceScope TimeScope("BackendCodegen", StringRef(""));
 CodeGenPasses.run(*TheModule);
   }
```

But that actually brings me to another question - what about 
`PrettyStackTraceString CrashInfo("Per-function optimization");`?
Should that be wrapped into some section, too?
I'm less certain here.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > anton-afanasyev wrote:
> > > > anton-afanasyev wrote:
> > > > > lebedev.ri wrote:
> > > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > > "Code Generation Time")` timer.
> > > > > Thanks, I'm to add it.
> > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > timings, i think? :)
> > > 
> > > I have noticed this because when i looked at the produced time flame 
> > > graph,
> > > there's large section in the end that is covered only by `"Backend"` 
> > > timer,
> > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > section
> > > should or should not be within `"Backend"` timer, but it certainly should 
> > > *also*
> > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > {F10062322}
> > > {F10062316}
> > "Mostly coincides" here means "identical" I believe, the difference is 
> > auxiliary stuff.
> > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
> > `"codegen"` one.
> Then we are talking about different things using the same name.
> There are two distinct codegen steps:
> 1. clang AST -> LLVM IR codegen
> (after that, all the opt passes run)
> 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> passes.
> 
> Those are *different* codegen's.
Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
"Backend".


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > anton-afanasyev wrote:
> > > anton-afanasyev wrote:
> > > > lebedev.ri wrote:
> > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > "Code Generation Time")` timer.
> > > > Thanks, I'm to add it.
> > > Hmm, I've figured out this isn't needed: such new timer mostly coincides 
> > > with "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is 
> > > bad example of doing right timing.
> > "Mostly coincides" may not be the best way to approach fine-grained 
> > timings, i think? :)
> > 
> > I have noticed this because when i looked at the produced time flame graph,
> > there's large section in the end that is covered only by `"Backend"` timer,
> > but nothing else. Now, i'm not going to say whether or not that extra 
> > section
> > should or should not be within `"Backend"` timer, but it certainly should 
> > *also*
> > be within `"codegen"` timer. Or is there no codegen time spent there?
> > {F10062322}
> > {F10062316}
> "Mostly coincides" here means "identical" I believe, the difference is 
> auxiliary stuff.
> Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
> `"codegen"` one.
Then we are talking about different things using the same name.
There are two distinct codegen steps:
1. clang AST -> LLVM IR codegen
(after that, all the opt passes run)
2. LLVM IR -> final assembly. This happens after all the opt middle-end passes.

Those are *different* codegen's.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > anton-afanasyev wrote:
> > > lebedev.ri wrote:
> > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > > > Generation Time")` timer.
> > > Thanks, I'm to add it.
> > Hmm, I've figured out this isn't needed: such new timer mostly coincides 
> > with "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad 
> > example of doing right timing.
> "Mostly coincides" may not be the best way to approach fine-grained timings, 
> i think? :)
> 
> I have noticed this because when i looked at the produced time flame graph,
> there's large section in the end that is covered only by `"Backend"` timer,
> but nothing else. Now, i'm not going to say whether or not that extra section
> should or should not be within `"Backend"` timer, but it certainly should 
> *also*
> be within `"codegen"` timer. Or is there no codegen time spent there?
> {F10062322}
> {F10062316}
"Mostly coincides" here means "identical" I believe, the difference is 
auxiliary stuff.
Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
`"codegen"` one.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > > Generation Time")` timer.
> > Thanks, I'm to add it.
> Hmm, I've figured out this isn't needed: such new timer mostly coincides with 
> "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad 
> example of doing right timing.
"Mostly coincides" may not be the best way to approach fine-grained timings, i 
think? :)

I have noticed this because when i looked at the produced time flame graph,
there's large section in the end that is covered only by `"Backend"` timer,
but nothing else. Now, i'm not going to say whether or not that extra section
should or should not be within `"Backend"` timer, but it certainly should *also*
be within `"codegen"` timer. Or is there no codegen time spent there?
{F10062322}
{F10062316}


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > Generation Time")` timer.
> Thanks, I'm to add it.
Hmm, I've figured out this isn't needed: such new timer mostly coincides with 
"Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad example 
of doing right timing.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-24 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> Generation Time")` timer.
Thanks, I'm to add it.



Comment at: llvm/trunk/lib/IR/LegacyPassManager.cpp:1686
 
+  llvm::TimeTraceScope TimeScope("OptModule", M.getName());
   for (Function &F : M)

lebedev.ri wrote:
> I think this may be the wrong place for this.
> This includes the entirety of the pipeline, including all of llvm back-end 
> stuff.
Ok, I'm just to delete this line, this block is outputted by 
`MPPassManager::runOnModule()` as well. This is just a section to see module 
name rather than to cover proper part of pipeline.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-09-19 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

This isn't covered by any timer; if you look in `BackendUtil.cpp`,
`EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
Generation Time")` timer.



Comment at: llvm/trunk/lib/IR/LegacyPassManager.cpp:1686
 
+  llvm::TimeTraceScope TimeScope("OptModule", M.getName());
   for (Function &F : M)

I think this may be the wrong place for this.
This includes the entirety of the pipeline, including all of llvm back-end 
stuff.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-16 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 10 inline comments as done.
anton-afanasyev added a comment.

Patch by @thakis for BE passes tracing: https://reviews.llvm.org/D60782


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-14 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

In D58675#1465706 , @anton-afanasyev 
wrote:

> In D58675#1465336 , @lebedev.ri 
> wrote:
>
> > Some post-commit review (please submit a new review, don't replace this 
> > diff)
> >  As usual, the incorrect license headers keep slipping through.
>
>
> Ok, I've made a separate review for this: https://reviews.llvm.org/D60663


Thank you.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-14 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 23 inline comments as done.
anton-afanasyev added a comment.

In D58675#1465336 , @lebedev.ri wrote:

> Some post-commit review (please submit a new review, don't replace this diff)
>  As usual, the incorrect license headers keep slipping through.


Ok, I've made a separate review for this: https://reviews.llvm.org/D60663




Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

lebedev.ri wrote:
> OOOPS, wrong license.
Yes, thanks.



Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:53
+/// is not initialized, the overhead is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(StringRef Name, StringRef Detail) {

lebedev.ri wrote:
> Did you mean to explicitly prohibit all the default constructors / 
> `operator=`?
Ok, I'm to add lines like `TimeTraceScope() = delete;` here.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

lebedev.ri wrote:
> Wrong license.
Yes, I'm to fix it.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C : Src) {

lebedev.ri wrote:
> lebedev.ri wrote:
> > `SmallString<32>` ?
> > Also, it is safe to `OS.reserve(Src.size())`
> Also, you probably want to add `raw_svector_ostream` ontop, and `<<` into it.
This function has been already dropped here https://reviews.llvm.org/D60609 . 
I'm to submit code without it if json library using are ok for performance.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:59-60
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};

lebedev.ri wrote:
> `SmallString<32>`?
Hmm, would it make a sense for `Detail`? `Entry`s are heap-allocated, the 
actual size is ranging from several bytes to several hundreds. Also, 
getQualifiedNameAsString() which is usually used for `Detail` returns 
std::string.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:70-71
+
+  void begin(std::string Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail()};
+Stack.push_back(std::move(E));

lebedev.ri wrote:
> Why not either take `StringRef` arg, or at least `std::move()` it when 
> creating `Entry`?
Do you mean `std::move(Name)`? Ok.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:89
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+  return Val.Name == E.Name;

lebedev.ri wrote:
> llvm::find_if(llvm::reverse(), ) 
Hmm, one need not `[rbegin(), rend()]`, but `[rbegin()++,rend()]`, so have to 
explicitly specify begin and end, `llvm::reverse(Stack)` is inappropriate here.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:99
+
+  void Write(std::unique_ptr &OS) {
+assert(Stack.empty() &&

lebedev.ri wrote:
> Why pass `std::unique_ptr` ?
> Just `raw_pwrite_stream &OS`
Yes, thanks! 
This was blindly copied from `CompilerInstance::createOutputFile()` return type.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:122
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),

lebedev.ri wrote:
> Elide `{}`
Ok, thanks.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:123
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration &A, const NameAndDuration &B) {

lebedev.ri wrote:
> llvm::sort <- this is a correctness issue.
Yes, thanks!



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:144-145
+
+  std::vector Stack;
+  std::vector Entries;
+  std::unordered_map TotalPerName;

lebedev.ri wrote:
> Would it make sense to make these `SmallVector`?
Ok, I'm to change it to
```
  SmallVector Stack;  
  SmallVector Entries;
```
`Stack` size may be enough for small sources while `Entries` usually amounts 
many thousands of `Entry`s.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:146-147
+  std::vector Entries;
+  std::unordered_map TotalPerName;
+  std::unordered_map CountPerName;
+  time_point StartTime;

lebedev.ri wrote:
> 1. Eww, `std::unordered_map`, that will have *horrible* perf.
> 2. Eww, map with key = string. Use `llvm::StringMap`
You are right, but this is already fixed and submitted in this follow-up: 
https://reviews.llvm.org/D60404



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:162
+

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-13 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C : Src) {

lebedev.ri wrote:
> `SmallString<32>` ?
> Also, it is safe to `OS.reserve(Src.size())`
Also, you probably want to add `raw_svector_ostream` ontop, and `<<` into it.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-13 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

Some post-commit review (please submit a new review, don't replace this diff)
As usual, the incorrect license headers keep slipping through.




Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

OOOPS, wrong license.



Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:53
+/// is not initialized, the overhead is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(StringRef Name, StringRef Detail) {

Did you mean to explicitly prohibit all the default constructors / `operator=`?



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

Wrong license.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C : Src) {

`SmallString<32>` ?
Also, it is safe to `OS.reserve(Src.size())`



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:59-60
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};

`SmallString<32>`?



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:70-71
+
+  void begin(std::string Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail()};
+Stack.push_back(std::move(E));

Why not either take `StringRef` arg, or at least `std::move()` it when creating 
`Entry`?



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:89
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+  return Val.Name == E.Name;

llvm::find_if(llvm::reverse(), ) 



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:99
+
+  void Write(std::unique_ptr &OS) {
+assert(Stack.empty() &&

Why pass `std::unique_ptr` ?
Just `raw_pwrite_stream &OS`



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:122
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),

Elide `{}`



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:123
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration &A, const NameAndDuration &B) {

llvm::sort <- this is a correctness issue.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:144-145
+
+  std::vector Stack;
+  std::vector Entries;
+  std::unordered_map TotalPerName;

Would it make sense to make these `SmallVector`?



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:146-147
+  std::vector Entries;
+  std::unordered_map TotalPerName;
+  std::unordered_map CountPerName;
+  time_point StartTime;

1. Eww, `std::unordered_map`, that will have *horrible* perf.
2. Eww, map with key = string. Use `llvm::StringMap`



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:162
+
+void timeTraceProfilerWrite(std::unique_ptr &OS) {
+  assert(TimeTraceProfilerInstance != nullptr &&

Just `raw_pwrite_stream &OS`?


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-12 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

Use native llvm JSON library update: https://reviews.llvm.org/D60609


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-04-08 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

Fix hashing update: https://reviews.llvm.org/D60404


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-30 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

In D58675#1449053 , @thakis wrote:

> ps: Hooray for landing this, and thanks for the cool feature!


Thanks! It's @aras_p accomplishment.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-30 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

In D58675#1449051 , @thakis wrote:

> Looks like this landed without tests. Please add tests in a follow-up.
>
> Also, it looks like the flag is currently a cc1 flag. This should probably be 
> a CoreOption instead so that both the gcc-style and the cl-style drivers 
> expose it. Right now, users can't really use this if I read the patch right 
> (without using -Xclang, which is discouraged.)


Ok, thanks, I'm to add tests in a follow-up.

It works like `clang -ftime-trace main.cpp` now. Not sure I know where 
CoreOption is, but I'm to figure out it, thanks.


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-30 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

ps: Hooray for landing this, and thanks for the cool feature!


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-30 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

Looks like this landed without tests. Please add tests in a follow-up.

Also, it looks like the flag is currently a cc1 flag. This should probably be a 
CoreOption instead so that both the gcc-style and the cl-style drivers expose 
it. Right now, users can't really use this if I read the patch right (without 
using -Xclang, which is discouraged.)


Repository:
  rL LLVM

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-30 Thread Anton Afanasyev via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357340: Adds `-ftime-trace` option to clang that produces 
Chrome `chrome://tracing`… (authored by anton-afanasyev, committed by ).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D58675?vs=192525&id=192963#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675

Files:
  cfe/trunk/include/clang/Basic/CodeGenOptions.def
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/FrontendOptions.h
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInstance.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Parse/ParseAST.cpp
  cfe/trunk/lib/Parse/ParseDeclCXX.cpp
  cfe/trunk/lib/Parse/ParseTemplate.cpp
  cfe/trunk/lib/Sema/Sema.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
  cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp
  cfe/trunk/tools/driver/cc1_main.cpp
  llvm/trunk/include/llvm/Support/TimeProfiler.h
  llvm/trunk/lib/IR/LegacyPassManager.cpp
  llvm/trunk/lib/Support/CMakeLists.txt
  llvm/trunk/lib/Support/TimeProfiler.cpp

Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -42,6 +42,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
@@ -1382,6 +1383,9 @@
   const llvm::DataLayout &TDesc, Module *M,
   BackendAction Action,
   std::unique_ptr OS) {
+
+  llvm::TimeTraceScope TimeScope("Backend", StringRef(""));
+
   std::unique_ptr EmptyModule;
   if (!CGOpts.ThinLTOIndexFile.empty()) {
 // If we are performing a ThinLTO importing compile, load the function index
Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -58,6 +58,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MD5.h"
+#include "llvm/Support/TimeProfiler.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -2482,6 +2483,9 @@
 if (!shouldEmitFunction(GD))
   return;
 
+llvm::TimeTraceScope TimeScope(
+"CodeGen Function", [&]() { return FD->getQualifiedNameAsString(); });
+
 if (const auto *Method = dyn_cast(D)) {
   // Make sure to emit the definition(s) before we emit the thunks.
   // This is necessary for the generation of certain thunks.
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4548,6 +4548,7 @@
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_print_source_range_info);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_parseable_fixits);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_report);
+  Args.AddLastArg(CmdArgs, options::OPT_ftime_trace);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
 
Index: cfe/trunk/lib/Sema/Sema.cpp
===
--- cfe/trunk/lib/Sema/Sema.cpp
+++ cfe/trunk/lib/Sema/Sema.cpp
@@ -39,6 +39,8 @@
 #include "clang/Sema/TemplateInstCallback.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/Support/TimeProfiler.h"
+
 using namespace clang;
 using namespace sema;
 
@@ -92,6 +94,12 @@
   SourceManager &SM = S->getSourceManager();
   SourceLocation IncludeLoc = SM.getIncludeLoc(SM.getFileID(Loc));
   if (IncludeLoc.isValid()) {
+if (llvm::timeTraceProfilerEnabled()) {
+  const FileEntry *FE = SM.getFileEntryForID(SM.getFileID(Loc));
+  llvm::timeTraceProfilerBegin(
+  "Source", FE != nullptr ? FE->getName() : StringRef(""));
+}
+
 IncludeStack.push_back(IncludeLoc);
 S->DiagnoseNonDefaultPragmaPack(
 Sema::PragmaPackDiagnoseKind::NonDefaultStateAtInclude, IncludeLoc);
@@ -99,10 +107,14 @@
   break;
 }
 case ExitFile:
-  if (!IncludeStack.empty())
+  if (!IncludeStack.empty()) {
+if (llvm::timeTraceProfilerEnabled())
+  llvm::timeTraceProfilerEnd();
+
 S->DiagnoseNonDefaultPragmaPack(
 Sema::PragmaPackDiagnoseKind::ChangedStateAtExit,
 Inclu

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-29 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

In D58675#1448112 , @rnk wrote:

> lgtm
>
> I think this is ready. We can adjust the overloads after the fact. I'd like 
> to get the feature in so we can make improvements independently.

Ok, so I'm to commit this and fix three points in the next commit(s):

1. Improve hashing (it's easy).
2. Use json utility.
3. Improve function_ref constructor and delete StringRef("...") explicit 
casting.

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-29 Thread Reid Kleckner via Phabricator via cfe-commits

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

I think this is ready. We can adjust the overloads after the fact. I'd like to 
get the feature in so we can make improvements independently.




Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
 P.Initialize();

anton-afanasyev wrote:
> takuto.ikuta wrote:
> > anton-afanasyev wrote:
> > > takuto.ikuta wrote:
> > > > Remove StringRef?
> > > I use `StringRef("")` to explicitly cast to one of overloaded 
> > > `TimeScope(StringRef, StringRef)` and `TimeScope(StringRef, 
> > > fuction_ref(std::string()))`.
> > I think function_ref(std::string()) does not have constructor receiving 
> > StringRef, so I feel explicit cast is redundant.
> > 
> The compiler tries to use function_ref<..>(Callable&&, ...) constructor and 
> instantiates `function_ref::function_ref`, 
> so one gets error like:
> ```
> error: call to constructor of 'llvm::TimeTraceScope' is ambiguous
> llvm::TimeTraceScope TimeScope("Frontend", "");
>  ^ ~~
> .../include/llvm/Support/TimeProfiler.h:54:3: note: candidate constructor
>   TimeTraceScope(StringRef Name, StringRef Detail) {
>   ^
> .../include/llvm/Support/TimeProfiler.h:58:3: note: candidate constructor
>   TimeTraceScope(StringRef Name, llvm::function_ref Detail) {
>   ^
> 
> ```
Oh no. We are too clever for ourselves. :(

So, we'd need to make the enable_if logic in the function_ref constructor more 
clever to prevent it from being instantiated for non-callables.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-27 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev updated this revision to Diff 192525.
anton-afanasyev marked 10 inline comments as done.

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

https://reviews.llvm.org/D58675

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/IR/LegacyPassManager.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -0,0 +1,185 @@
+//===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+/// \file Hierarchical time profiler implementation.
+//
+//===--===//
+
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/FileSystem.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace std::chrono;
+
+namespace llvm {
+
+TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C: Src) {
+switch (C) {
+case '"':
+case '/':
+case '\\':
+case '\b':
+case '\f':
+case '\n':
+case '\r':
+case '\t':
+  OS += '\\';
+  OS += C;
+  break;
+default:
+  if (isPrint(C)) {
+OS += C;
+  }
+}
+  }
+  return OS;
+}
+
+typedef duration DurationType;
+typedef std::pair NameAndDuration;
+
+struct Entry {
+  time_point Start;
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};
+
+struct TimeTraceProfiler {
+  TimeTraceProfiler() {
+Stack.reserve(8);
+Entries.reserve(128);
+StartTime = steady_clock::now();
+  }
+
+  void begin(std::string Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail()};
+Stack.push_back(std::move(E));
+  }
+
+  void end() {
+assert(!Stack.empty() && "Must call begin() first");
+auto &E = Stack.back();
+E.Duration = steady_clock::now() - E.Start;
+
+// Only include sections longer than 500us.
+if (duration_cast(E.Duration).count() > 500)
+  Entries.emplace_back(E);
+
+// Track total time taken by each "name", but only the topmost levels of
+// them; e.g. if there's a template instantiation that instantiates other
+// templates from within, we only want to add the topmost one. "topmost"
+// happens to be the ones that don't have any currently open entries above
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+  return Val.Name == E.Name;
+}) == Stack.rend()) {
+  TotalPerName[E.Name] += E.Duration;
+  CountPerName[E.Name]++;
+}
+
+Stack.pop_back();
+  }
+
+  void Write(std::unique_ptr &OS) {
+assert(Stack.empty() &&
+   "All profiler sections should be ended when calling Write");
+
+*OS << "{ \"traceEvents\": [\n";
+
+// Emit all events for the main flame graph.
+for (const auto &E : Entries) {
+  auto StartUs = duration_cast(E.Start - StartTime).count();
+  auto DurUs = duration_cast(E.Duration).count();
+  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+  << ", \"dur\":" << DurUs << ", \"name\":\""
+  << escapeString(E.Name) << "\", \"args\":{ \"detail\":\""
+  << escapeString(E.Detail) << "\"} },\n";
+}
+
+// Emit totals by section name as additional "thread" events, sorted from
+// longest one.
+int Tid = 1;
+std::vector SortedTotals;
+SortedTotals.reserve(TotalPerName.size());
+for (const auto &E : TotalPerName) {
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration &A, const NameAndDuration &B) {
+return A.second > B.second;
+  });
+for (const auto &E : SortedTotals) {
+  auto DurUs = duration_cast(E.second).count();
+  *OS << "{ \"pid\":1, \"tid\":" << Tid << ", \"ph\"

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-27 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked an inline comment as done.
anton-afanasyev added inline comments.



Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
 P.Initialize();

takuto.ikuta wrote:
> anton-afanasyev wrote:
> > takuto.ikuta wrote:
> > > Remove StringRef?
> > I use `StringRef("")` to explicitly cast to one of overloaded 
> > `TimeScope(StringRef, StringRef)` and `TimeScope(StringRef, 
> > fuction_ref(std::string()))`.
> I think function_ref(std::string()) does not have constructor receiving 
> StringRef, so I feel explicit cast is redundant.
> 
The compiler tries to use function_ref<..>(Callable&&, ...) constructor and 
instantiates `function_ref::function_ref`, so 
one gets error like:
```
error: call to constructor of 'llvm::TimeTraceScope' is ambiguous
llvm::TimeTraceScope TimeScope("Frontend", "");
 ^ ~~
.../include/llvm/Support/TimeProfiler.h:54:3: note: candidate constructor
  TimeTraceScope(StringRef Name, StringRef Detail) {
  ^
.../include/llvm/Support/TimeProfiler.h:58:3: note: candidate constructor
  TimeTraceScope(StringRef Name, llvm::function_ref Detail) {
  ^

```


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-27 Thread Takuto Ikuta via Phabricator via cfe-commits

takuto.ikuta added inline comments.



Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
 P.Initialize();

anton-afanasyev wrote:
> takuto.ikuta wrote:
> > Remove StringRef?
> I use `StringRef("")` to explicitly cast to one of overloaded 
> `TimeScope(StringRef, StringRef)` and `TimeScope(StringRef, 
> fuction_ref(std::string()))`.
I think function_ref(std::string()) does not have constructor receiving 
StringRef, so I feel explicit cast is redundant.




Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+*OS << "{ \"traceEvents\": [\n";
+

anton-afanasyev wrote:
> takuto.ikuta wrote:
> > Don't we want to use json utility for this?
> > https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/JSON.h
> > 
> This implementation looks compact and fast. I've read proposal for this 
> library 
> https://docs.google.com/document/d/1OEF9IauWwNuSigZzvvbjc1cVS1uGHRyGTXaoy3DjqM4
>  , it's recent, so I'm not sure it's stable and speed optimized. Do you 
> actually can advise it? I can do it in the next refactor commit as well.
Hmm, I think using json library is preferred because we don't need to take care 
the correctness of json format.
Confirming correctness of json format with many escaped literals is bit hard 
and I'm afraid to miss json format error.



Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+default:
+  if (isPrint(C)) {
+OS += C;

anton-afanasyev wrote:
> takuto.ikuta wrote:
> > include StringExtras.h for this?
> Should one do it? It's already implicitly included.
I think it is better to do, because if someone removes the StringExtras.h 
include used for this file, they need to include header in this file at the 
same time. That may require to touch unrelated files in their change.



Comment at: llvm/lib/Support/TimeProfiler.cpp:75
+  void end() {
+assert(!Stack.empty() && "Must call Begin first");
+auto &E = Stack.back();

s/Begin/begin/


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-27 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added inline comments.



Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
 P.Initialize();

takuto.ikuta wrote:
> Remove StringRef?
I use `StringRef("")` to explicitly cast to one of overloaded 
`TimeScope(StringRef, StringRef)` and `TimeScope(StringRef, 
fuction_ref(std::string()))`.



Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+*OS << "{ \"traceEvents\": [\n";
+

takuto.ikuta wrote:
> Don't we want to use json utility for this?
> https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/JSON.h
> 
This implementation looks compact and fast. I've read proposal for this library 
https://docs.google.com/document/d/1OEF9IauWwNuSigZzvvbjc1cVS1uGHRyGTXaoy3DjqM4 
, it's recent, so I'm not sure it's stable and speed optimized. Do you actually 
can advise it? I can do it in the next refactor commit as well.



Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+default:
+  if (isPrint(C)) {
+OS += C;

takuto.ikuta wrote:
> include StringExtras.h for this?
Should one do it? It's already implicitly included.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-26 Thread Takuto Ikuta via Phabricator via cfe-commits

takuto.ikuta added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1352
+
+  llvm::TimeTraceScope TimeScope("Backend", StringRef(""));
+

We don't need explicit StringRef constructor call here. Just passing "" is 
enough.



Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
 P.Initialize();

Remove StringRef?



Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:131
   NumIdentifierLookupHits() {
+  llvm::TimeTraceScope TimeScope("Module LoadIndex", StringRef(""));
   // Read the global index.

Remove StringRef?



Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:745
   using namespace llvm;
+  llvm::TimeTraceScope TimeScope("Module WriteIndex", StringRef(""));
 

Remove StringRef?



Comment at: clang/tools/driver/cc1_main.cpp:224
+  {
+llvm::TimeTraceScope TimeScope("ExecuteCompiler", StringRef(""));
+Success = ExecuteCompilerInvocation(Clang.get());

Remove StringRef?



Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+default:
+  if (isPrint(C)) {
+OS += C;

include StringExtras.h for this?


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-26 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev updated this revision to Diff 192268.
anton-afanasyev marked an inline comment as done.

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

https://reviews.llvm.org/D58675

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/IR/LegacyPassManager.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -0,0 +1,184 @@
+//===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+/// \file Hierarchical time profiler implementation.
+//
+//===--===//
+
+#include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/FileSystem.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace std::chrono;
+
+namespace llvm {
+
+TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char &C: Src) {
+switch (C) {
+case '"':
+case '/':
+case '\\':
+case '\b':
+case '\f':
+case '\n':
+case '\r':
+case '\t':
+  OS += '\\';
+  OS += C;
+  break;
+default:
+  if (isPrint(C)) {
+OS += C;
+  }
+}
+  }
+  return OS;
+}
+
+typedef duration DurationType;
+typedef std::pair NameAndDuration;
+
+struct Entry {
+  time_point Start;
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};
+
+struct TimeTraceProfiler {
+  TimeTraceProfiler() {
+Stack.reserve(8);
+Entries.reserve(128);
+StartTime = steady_clock::now();
+  }
+
+  void begin(std::string Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail()};
+Stack.push_back(std::move(E));
+  }
+
+  void end() {
+assert(!Stack.empty() && "Must call Begin first");
+auto &E = Stack.back();
+E.Duration = steady_clock::now() - E.Start;
+
+// Only include sections longer than 500us.
+if (duration_cast(E.Duration).count() > 500)
+  Entries.emplace_back(E);
+
+// Track total time taken by each "name", but only the topmost levels of
+// them; e.g. if there's a template instantiation that instantiates other
+// templates from within, we only want to add the topmost one. "topmost"
+// happens to be the ones that don't have any currently open entries above
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+  return Val.Name == E.Name;
+}) == Stack.rend()) {
+  TotalPerName[E.Name] += E.Duration;
+  CountPerName[E.Name]++;
+}
+
+Stack.pop_back();
+  }
+
+  void Write(std::unique_ptr &OS) {
+assert(Stack.empty() &&
+   "All profiler sections should be ended when calling Write");
+
+*OS << "{ \"traceEvents\": [\n";
+
+// Emit all events for the main flame graph.
+for (const auto &E : Entries) {
+  auto StartUs = duration_cast(E.Start - StartTime).count();
+  auto DurUs = duration_cast(E.Duration).count();
+  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+  << ", \"dur\":" << DurUs << ", \"name\":\""
+  << escapeString(E.Name) << "\", \"args\":{ \"detail\":\""
+  << escapeString(E.Detail) << "\"} },\n";
+}
+
+// Emit totals by section name as additional "thread" events, sorted from
+// longest one.
+int Tid = 1;
+std::vector SortedTotals;
+SortedTotals.reserve(TotalPerName.size());
+for (const auto &E : TotalPerName) {
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration &A, const NameAndDuration &B) {
+return A.second > B.second;
+  });
+for (const auto &E : SortedTotals) {
+  auto DurUs = duration_cast(E.second).count();
+  *OS << "{ \"pid\":1, \"tid\":" << Tid << ", \"ph\":\"X\", \"ts\":" << 0
+  << ",

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-26 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 8 inline comments as done.
anton-afanasyev added inline comments.



Comment at: llvm/lib/Support/TimeProfiler.cpp:33
+switch (C) {
+case '"':
+case '\\':

takuto.ikuta wrote:
> Include escape for '/' too.
> https://tools.ietf.org/html/rfc8259#section-7
Yes, 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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-23 Thread Takuto Ikuta via Phabricator via cfe-commits

takuto.ikuta added inline comments.



Comment at: llvm/lib/Support/TimeProfiler.cpp:28
+
+static std::string escapeString(const char *Src) {
+  std::string OS;

you can pass StringRef here and remove .data() or .c_str() from caller.



Comment at: llvm/lib/Support/TimeProfiler.cpp:33
+switch (C) {
+case '"':
+case '\\':

Include escape for '/' too.
https://tools.ietf.org/html/rfc8259#section-7



Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+default:
+  if (C >= 32 && C < 126) {
+OS += C;

I prefer to use isPrint here.
https://github.com/llvm/llvm-project/blob/2946cd701067404b99c39fb29dc9c74bd7193eb3/llvm/include/llvm/ADT/StringExtras.h#L105



Comment at: llvm/lib/Support/TimeProfiler.cpp:72
+Entry E = {steady_clock::now(), {}, Name, Detail};
+Stack.emplace_back(E);
+  }

I prefer to write
```
Stack.push_back(std::move(E));
```
or
```
Stack.emplace_back(steady_clock::now(), {}, Name, Detail);
```



Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+*OS << "{ \"traceEvents\": [\n";
+

Don't we want to use json utility for this?
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/JSON.h




Comment at: llvm/lib/Support/TimeProfiler.cpp:147
+  std::unordered_map TotalPerName;
+  std::unordered_map CountPerName;
+  time_point StartTime;

better to have one hash map so that we don't need to call 2 lookup in L92 and 
L93.
Also StringMap may be faster than unordered_map.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-22 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

Other than the lifetime issue, I think this is basically ready.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2014
+  llvm::TimeTraceScope TimeScope("InstantiateClass", [&]() {
+  return Instantiation->getQualifiedNameAsString().c_str();
+});

Shoot, I think the callable should probably return std::string instead of 
StringRef, otherwise this looks like it will be a use-after-free. You allocate 
a temporary std::string, get a pointer to the characters, return, the string is 
destroyed, and then UAF.

With a std::string return type, you won't need `.c_str()`.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4089
+  llvm::TimeTraceScope TimeScope("InstantiateFunction", [&]() {
+  return Function->getQualifiedNameAsString().c_str();
+});

`.c_str()` is unnecessary



Comment at: llvm/include/llvm/Support/TimeProfiler.h:54-58
+  TimeTraceScope(const char *Name, const char *Detail) {
+if (TimeTraceProfilerInstance != nullptr)
+  timeTraceProfilerBegin(Name, Detail);
+  }
+  TimeTraceScope(const char *Name, llvm::function_ref Detail) {

I think you can replace `const char *` in these prototypes with StringRef to 
save a few `.data()` calls at some call sites.

As mentioned before, I think the callable needs to return `std::string`, though.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-22 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev updated this revision to Diff 191934.
anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added a comment.

Updated following @rnk review notes.


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

https://reviews.llvm.org/D58675

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/IR/LegacyPassManager.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -0,0 +1,185 @@
+//===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+/// \file Hierarchical time profiler implementation.
+//
+//===--===//
+
+#include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/FileSystem.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace std::chrono;
+
+namespace llvm {
+
+TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
+
+static std::string escapeString(const char *Src) {
+  std::string OS;
+  while (*Src) {
+char C = *Src;
+switch (C) {
+case '"':
+case '\\':
+case '\b':
+case '\f':
+case '\n':
+case '\r':
+case '\t':
+  OS += '\\';
+  OS += C;
+  break;
+default:
+  if (C >= 32 && C < 126) {
+OS += C;
+  }
+}
+++Src;
+  }
+  return OS;
+}
+
+typedef duration DurationType;
+typedef std::pair NameAndDuration;
+
+struct Entry {
+  time_point Start;
+  DurationType Duration;
+  std::string Name;
+  llvm::function_ref Detail;
+};
+
+struct TimeTraceProfiler {
+  TimeTraceProfiler() {
+Stack.reserve(8);
+Entries.reserve(128);
+StartTime = steady_clock::now();
+  }
+
+  void begin(const std::string &Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail};
+Stack.emplace_back(E);
+  }
+
+  void end() {
+assert(!Stack.empty() && "Must call Begin first");
+auto &E = Stack.back();
+E.Duration = steady_clock::now() - E.Start;
+
+// Only include sections longer than 500us.
+if (duration_cast(E.Duration).count() > 500)
+  Entries.emplace_back(E);
+
+// Track total time taken by each "name", but only the topmost levels of
+// them; e.g. if there's a template instantiation that instantiates other
+// templates from within, we only want to add the topmost one. "topmost"
+// happens to be the ones that don't have any currently open entries above
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry &Val) {
+  return Val.Name == E.Name;
+}) == Stack.rend()) {
+  TotalPerName[E.Name] += E.Duration;
+  CountPerName[E.Name]++;
+}
+
+Stack.pop_back();
+  }
+
+  void Write(std::unique_ptr &OS) {
+assert(Stack.empty() &&
+   "All profiler sections should be ended when calling Write");
+
+*OS << "{ \"traceEvents\": [\n";
+
+// Emit all events for the main flame graph.
+for (const auto &E : Entries) {
+  auto StartUs = duration_cast(E.Start - StartTime).count();
+  auto DurUs = duration_cast(E.Duration).count();
+  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+  << ", \"dur\":" << DurUs << ", \"name\":\""
+  << escapeString(E.Name.c_str()) << "\", \"args\":{ \"detail\":\""
+  << escapeString(E.Detail().str().c_str()) << "\"} },\n";
+}
+
+// Emit totals by section name as additional "thread" events, sorted from
+// longest one.
+int Tid = 1;
+std::vector SortedTotals;
+SortedTotals.reserve(TotalPerName.size());
+for (const auto &E : TotalPerName) {
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration &A, const NameAndDuration &B) {
+return A.second > B.second;
+  });
+for (const auto &E : SortedTotals) {
+  auto DurUs = duration_cast(E.second)

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-22 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 14 inline comments as done.
anton-afanasyev added inline comments.



Comment at: llvm/lib/IR/LegacyPassManager.cpp:1632-1634
+  bool profileTime = llvm::TimeTraceProfilerEnabled();
+  if (profileTime)
+llvm::TimeTraceProfilerBegin("OptFunction", F.getName().data());

anton-afanasyev wrote:
> 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.
Looks like PassManager.h changing is enough to support new PassManager. But I 
prefer to make this change in subsequent commits.



Comment at: llvm/lib/IR/LegacyPassManager.cpp:1686
 
+  llvm::TimeTraceScope timeScope("OptModule", M.getName().data());
   for (Function &F : M)

rnk wrote:
> I think these OptModule and OptFunction labels may need some improvement. For 
> a backend-heavy compilation like LLVM's X86ISelLowering.cpp, these labels 
> aren't as good as they could be: {F8455962}
> I think it's fine to leave that for later, though.
Yes, leaving this for later. The subsequent commits are planned.



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};

rnk wrote:
> 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.
Ok, I'm not changing this.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-14 Thread Anton Afanasyev via Phabricator via cfe-commits

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-14 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added inline comments.



Comment at: llvm/lib/IR/LegacyPassManager.cpp:1686
 
+  llvm::TimeTraceScope timeScope("OptModule", M.getName().data());
   for (Function &F : M)

I think these OptModule and OptFunction labels may need some improvement. For a 
backend-heavy compilation like LLVM's X86ISelLowering.cpp, these labels aren't 
as good as they could be: {F8455962}
I think it's fine to leave that for later, though.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-14 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added inline comments.



Comment at: llvm/lib/Support/TimeProfiler.h:1
+//===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ 
-*-===//
+//

I applied this patch locally to try it, and I noticed this header should be in 
llvm/include/llvm/Support, not llvm/lib/Support.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-13 Thread Reid Kleckner via Phabricator via cfe-commits

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(TagDecl)
+  ? cast(TagDecl)->getQualifiedNameAsString().data()
+  : ""));

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()
+ : ""));

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(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 detail` 
overload. The callable also allows the user to bind variables like this:
  TimeTraceScope timeScope("ParseTag", [&]() {
if (auto *TD = dyn_cast_or_null(D))
  return TD->getNameAsStr();
return "";
  });
Instead of the `isa(D) ? cast(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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-13 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

Very cool! I'll take a look, I wasn't aware this had been rebased and uploaded, 
I was thinking about doing it myself yesterday as a side project.

As I think I've said elsewhere, I'm really excited to give users the tools they 
need to analyze why their code compiles slowly. If we just give them the tools, 
maybe they will restructure their code on their own and they won't come asking, 
"why is the compiler so slow on my (many transitive includes | template 
metaprogram)?".

I'll take a look and try to get back with some thoughts.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-13 Thread Zachary Turner via Phabricator via cfe-commits

zturner added reviewers: rnk, hans.
zturner added a comment.

+reid and hans, as they might be interested in this.


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-13 Thread Simon Pilgrim via Phabricator via cfe-commits

RKSimon added a comment.

@rsmith Any suggestions for good reviewers for this please?


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-12 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev added a comment.

Ping!
Should I add more FE guys to review this?


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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-03-01 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev updated this revision to Diff 11.

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

https://reviews.llvm.org/D58675

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/lib/IR/LegacyPassManager.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/TimeProfiler.h

Index: llvm/lib/Support/TimeProfiler.h
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.h
@@ -0,0 +1,71 @@
+//===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_SUPPORT_TIME_PROFILER_H
+#define LLVM_SUPPORT_TIME_PROFILER_H
+
+#include "llvm/Support/raw_ostream.h"
+
+namespace llvm {
+
+struct TimeTraceProfiler;
+extern TimeTraceProfiler *TimeTraceProfilerInstance;
+
+/// Initialize the time trace profiler.
+/// This sets up the global \p TimeTraceProfilerInstance
+/// variable to be the profiler instance.
+void TimeTraceProfilerInitialize();
+
+/// Cleanup the time trace profiler, if it was initialized.
+void TimeTraceProfilerCleanup();
+
+/// Is the time trace profiler enabled, i.e. initialized?
+inline bool TimeTraceProfilerEnabled() {
+  return TimeTraceProfilerInstance != nullptr;
+}
+
+/// Write profiling data to output file.
+/// Data produced is JSON, in Chrome "Trace Event" format, see
+/// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
+void TimeTraceProfilerWrite(std::unique_ptr &OS);
+
+/// Manually begin a time section, with the given \p name and \p detail.
+/// Profiler copies the string data, so the pointers can be given into
+/// temporaries. Time sections can be hierarchical; every Begin must have a
+/// matching End pair but they can nest.
+void TimeTraceProfilerBegin(const char *name, const char *detail);
+
+/// Manually end the last time section.
+void TimeTraceProfilerEnd();
+
+/// The TimeTraceScope is a helper class to call the begin and end functions.
+/// of the time trace profiler.  When the object is constructed, it
+/// begins the section; and wen it is destroyed, it stops
+/// it.  If the time profiler is not initialized, the overhead
+/// is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(const char *name, const char *detail) {
+if (TimeTraceProfilerInstance != nullptr)
+  TimeTraceProfilerBegin(name, detail);
+  }
+  ~TimeTraceScope() {
+if (TimeTraceProfilerInstance != nullptr)
+  TimeTraceProfilerEnd();
+  }
+};
+
+/// Evaluates expression if time trace profiler is enabled, or passed null when
+/// it is not. Useful to avoid possibly expensive work in creating a string for
+/// profiling, when profiler is not enabled at all.
+#define TIME_TRACE_OR_NULL(expr)   \
+  (llvm::TimeTraceProfilerInstance != nullptr ? (expr) : nullptr)
+
+} // end namespace llvm
+
+#endif
Index: llvm/lib/Support/TimeProfiler.cpp
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -0,0 +1,178 @@
+//===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+/// \file Hierarchical time profiler implementation.
+//
+//===--===//
+
+#include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/FileSystem.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace std::chrono;
+
+namespace llvm {
+
+TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
+
+static std::string EscapeString(const char *src) {
+  std::string os;
+  while (*src) {
+char c = *src;
+switch (c) {
+case '"':
+case '\\':
+case '\b':
+case '\f':
+case '\n':
+case '\r':
+case '\t':
+  os += '\\';
+  os += c;
+  break;
+default:
+  if (c >= 32 && c < 126) {
+os

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-02-28 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked an inline comment as done.
anton-afanasyev added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:23
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"

RKSimon wrote:
> anton-afanasyev wrote:
> > RKSimon wrote:
> > > nfc change?
> > What do you mean? This change is not NFC.
> Moving the "clang/Sema/SemaInternal.h" include?
Ok, I'm to move this line back. This was done by clang-format.


Repository:
  rC Clang

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-02-28 Thread Simon Pilgrim via Phabricator via cfe-commits

RKSimon added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:23
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"

anton-afanasyev wrote:
> RKSimon wrote:
> > nfc change?
> What do you mean? This change is not NFC.
Moving the "clang/Sema/SemaInternal.h" include?


Repository:
  rC Clang

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-02-28 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added a comment.

Ok, I'm to add tests and reviewers.




Comment at: clang/lib/Sema/Sema.cpp:113
+  llvm::TimeTraceProfilerEnd();
+}
 S->DiagnoseNonDefaultPragmaPack(

RKSimon wrote:
> remove braces
Ok



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:23
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"

RKSimon wrote:
> nfc change?
What do you mean? This change is not NFC.



Comment at: clang/tools/driver/cc1_main.cpp:201
+llvm::TimeTraceProfilerInitialize();
+  }
+

RKSimon wrote:
> remove braces
Ok


Repository:
  rC Clang

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-02-28 Thread Simon Pilgrim via Phabricator via cfe-commits

RKSimon added a comment.

Ideally I think you need more clang/frontend experts reviewers - many of us on 
the reviewers list tend to work mainly in llvm.

Test cases would be good as well - even if its just basic sanity tests for 
command line args etc.




Comment at: clang/lib/Sema/Sema.cpp:113
+  llvm::TimeTraceProfilerEnd();
+}
 S->DiagnoseNonDefaultPragmaPack(

remove braces



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:24
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"

nfc change?



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:23
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"

nfc change?



Comment at: clang/tools/driver/cc1_main.cpp:201
+llvm::TimeTraceProfilerInitialize();
+  }
+

remove braces


Repository:
  rC Clang

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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

2019-02-26 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev created this revision.
Herald added subscribers: llvm-commits, cfe-commits, jdoerfert, arphaman, 
mgrang, hiraditya, mgorny, mehdi_amini.
Herald added projects: clang, LLVM.

This is the first part of time tracing system, I have splitted them cause this 
part is mostly written by Aras Pranckevicius except of several minor fixes 
concerning formatting. I'm to commit this in behalf of Aras, we have an 
arrangment with him.
The second part extends this option adding terminal output making no need for 
profiling visualization. I can also add xray support though this need is 
arguable.
The third part is for cleaning up previous attempts of such implementations 
(like https://reviews.llvm.org/D45619, https://reviews.llvm.org/D43578 and 
https://reviews.llvm.org/D47196).

This is taken from GitHub PR: 
https://github.com/aras-p/llvm-project-20170507/pull/2
Here is cfe maillist subject discussion: 
http://lists.llvm.org/pipermail/cfe-dev/2019-January/060836.html


Repository:
  rC Clang

https://reviews.llvm.org/D58675

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/tools/driver/cc1_main.cpp
  llvm/lib/IR/LegacyPassManager.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/TimeProfiler.h

Index: llvm/lib/Support/TimeProfiler.h
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.h
@@ -0,0 +1,71 @@
+//===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_SUPPORT_TIME_PROFILER_H
+#define LLVM_SUPPORT_TIME_PROFILER_H
+
+#include "llvm/Support/raw_ostream.h"
+
+namespace llvm {
+
+struct TimeTraceProfiler;
+extern TimeTraceProfiler *TimeTraceProfilerInstance;
+
+/// Initialize the time trace profiler.
+/// This sets up the global \p TimeTraceProfilerInstance
+/// variable to be the profiler instance.
+void TimeTraceProfilerInitialize();
+
+/// Cleanup the time trace profiler, if it was initialized.
+void TimeTraceProfilerCleanup();
+
+/// Is the time trace profiler enabled, i.e. initialized?
+inline bool TimeTraceProfilerEnabled() {
+  return TimeTraceProfilerInstance != nullptr;
+}
+
+/// Write profiling data to output file.
+/// Data produced is JSON, in Chrome "Trace Event" format, see
+/// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
+void TimeTraceProfilerWrite(std::unique_ptr &OS);
+
+/// Manually begin a time section, with the given \p name and \p detail.
+/// Profiler copies the string data, so the pointers can be given into
+/// temporaries. Time sections can be hierarchical; every Begin must have a
+/// matching End pair but they can nest.
+void TimeTraceProfilerBegin(const char *name, const char *detail);
+
+/// Manually end the last time section.
+void TimeTraceProfilerEnd();
+
+/// The TimeTraceScope is a helper class to call the begin and end functions.
+/// of the time trace profiler.  When the object is constructed, it
+/// begins the section; and wen it is destroyed, it stops
+/// it.  If the time profiler is not initialized, the overhead
+/// is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(const char *name, const char *detail) {
+if (TimeTraceProfilerInstance != nullptr)
+  TimeTraceProfilerBegin(name, detail);
+  }
+  ~TimeTraceScope() {
+if (TimeTraceProfilerInstance != nullptr)
+  TimeTraceProfilerEnd();
+  }
+};
+
+/// Evaluates expression if time trace profiler is enabled, or passed null when
+/// it is not. Useful to avoid possibly expensive work in creating a string for
+/// profiling, when profiler is not enabled at all.
+#define TIME_TRACE_OR_NULL(expr)   \
+  (llvm::TimeTraceProfilerInstance != nullptr ? (expr) : nullptr)
+
+} // end namespace llvm
+
+#endif
Index: llvm/lib/Support/TimeProfiler.cpp
===
--- /dev/null
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -0,0 +1,178 @@
+//===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University

49 matches

Mail list logo