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

[Kim Gräsman via cfe-commits]

Hi Richard,

On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:
> rsmith added a comment.
>
> So I don't think this patch is reasonable for that reason. I'm also not sure 
> whether this, as is, is addressing a pressing use case -- for Clang 
> developers, existing non-invasive profiling tools (such as linux-perftools) 
> are likely to work a lot better for identifying where in the Clang source 
> code we're spending time. And Clang users typically don't care which function 
> we're in (unless they're filing a bug, where again a profiler is probably a 
> better tool).
>
> However, I do think we could make `-ftime-report` vastly more useful to Clang 
> users. Specifically, I think the useful, actionable feedback we can give to 
> users would be to tell them which parts of //their source code// are taking a 
> long time to compile. Profiling information that can describe -- for instance 
> -- the time spent instantiating the N slowest templates, or handling the N 
> slowest functions, or evaluating the N slowest constant expressions, or 
> parsing the N slowest `#include`d files, seems like it would be incredibly 
> valuable. To make that work, I think we'll need to teach LLVM's timer 
> infrastructure to properly separate out "self" time from "children" time for 
> timers in the same group, and may need other infrastructure improvements.

I disagreed up until the last paragraph :)

That's exactly the crux of what most users need to know -- which parts
of my source code are causing the biggest build slow-down? The summary
information from -ftime-report can give a hint, but a detailed
breakdown would of course be great!

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via cfe-commits]

On Wed, Apr 11, 2018 at 6:18 AM, Andrew V. Tischenko via Phabricator via
cfe-commits  wrote:

> avt77 added a comment.
>
> In https://reviews.llvm.org/D43578#1062950, @thakis wrote:
>
> > @davezarzycki remarks in https://reviews.llvm.org/D45485 that this
> breaks the shared build. The proposed fix there is to make several of
> clang's modules depend on LLVM's IR library ("Core"). This seems weird to
> me for two reasons, one architectural, one form a build point of view:
> >
> > 1. The modules growing the dep don't really depend on IR, they just need
> that one bool that happens to be defined there. That bool is called
> `TimePassesIsEnabled` which is a reasonable bool to live in IR, but this
> patch starts using that bool for meanings other than "should we time
> passes?". It instead uses the same bool to decide if clang should print a
> bunch of timing info. We probably should have a separate bool in clang and
> key this off that and make -ftime-report set both.
> > 2. From a build PoV, depending on Core means explicitly depending on
> TableGen processing the Attributes.td and Intrinsics.td files in
> include/llvm/IR, which needlessly (explicitly) serializes the build.
>
>
> In fact the current trunk already depends on TimePassesIsEnabled (w/o this
> patch applied):
>
> //$ find clang -name \*.cpp | xargs grep TimePassesIsEnabled
> clang/lib/CodeGen/CodeGenAction.cpp:  llvm::TimePassesIsEnabled =
> TimePasses;
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled)
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled)
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled) {
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled) {
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled)
> clang/lib/CodeGen/CodeGenAction.cpp:  if (llvm::TimePassesIsEnabled)
> clang/lib/CodeGen/CodeGenAction.cpp:if
> (llvm::TimePassesIsEnabled) {
> clang/lib/CodeGen/CodeGenAction.cpp:if
> (llvm::TimePassesIsEnabled) {
> clang/lib/CodeGen/BackendUtil.cpp:  TimeRegion 
> Region(llvm::TimePassesIsEnabled
> ?  : nullptr);
> clang/lib/CodeGen/BackendUtil.cpp:  TimeRegion 
> Region(llvm::TimePassesIsEnabled
> ?  : nullptr);
>

Note that these are all in CodeGen, which needs to depend on LLVM's IR
library anyway for code generation. It's still possible that CodeGen is
misusing TimePassesIsEnabled for a meaning which isn't "should we time
passes", in which case I agree we should change that, but at least it
doesn't add an unnecessary dependency there.


> //
> But I agree that such dependence is not OK. I'll create a separate bool
> instead of TimePassesIsEnabled but the question is should I remove the
> above usage of TimePassesIsEnabled as well? Or maybe it should be a
> separate pre-patch? Or it could be left as it is?
>

It could be a separate patch after your main change made it in.


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D43578
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits