<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 11, 2018 at 6:18 AM, Andrew V. Tischenko via Phabricator via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">avt77 added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D43578#1062950" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D43578#1062950</a>, @thakis wrote:<br>
<br>
> @davezarzycki remarks in <a href="https://reviews.llvm.org/D45485" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D45485</a> 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:<br>
><br>
> 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.<br>
> 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.<br>
<br>
<br>
In fact the current trunk already depends on TimePassesIsEnabled (w/o this patch applied):<br>
<br>
//$ find clang -name \*.cpp | xargs grep TimePassesIsEnabled<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      llvm::TimePassesIsEnabled = TimePasses;<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled)<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled)<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled) {<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled) {<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled)<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:      if (llvm::TimePassesIsEnabled)<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:        if (llvm::TimePassesIsEnabled) {<br>
clang/lib/CodeGen/<wbr>CodeGenAction.cpp:        if (llvm::TimePassesIsEnabled) {<br>
clang/lib/CodeGen/BackendUtil.<wbr>cpp:  TimeRegion Region(llvm::<wbr>TimePassesIsEnabled ? &CodeGenerationTime : nullptr);<br>
clang/lib/CodeGen/BackendUtil.<wbr>cpp:  TimeRegion Region(llvm::<wbr>TimePassesIsEnabled ? &CodeGenerationTime : nullptr);<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
//<br>
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?<br></blockquote><div><br></div><div>It could be a separate patch after your main change made it in.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D43578" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D43578</a><br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>