On Wed, Apr 11, 2018 at 6:18 AM, Andrew V. Tischenko via Phabricator via cfe-commits <cfe-commits@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@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
