[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 <cfe-commits at lists.llvm.org> 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
> ? &CodeGenerationTime : nullptr);
> clang/lib/CodeGen/BackendUtil.cpp:  TimeRegion Region(llvm::TimePassesIsEnabled
> ? &CodeGenerationTime : 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180411/30c5a5e9/attachment.html>