<div dir="ltr"><div><div>The clang patch lgtm, and I had some comments on the LLVM patch. Duncan, do you want to say more there?</div><div><br></div><div><div>--- include/llvm/Transforms/IPO/PassManagerBuilder.h<span class="" style="white-space:pre"> </span>(revision 237590)</div><div>+++ include/llvm/Transforms/IPO/PassManagerBuilder.h<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -121,6 +121,7 @@ class PassManagerBuilder {</div><div> bool VerifyInput;</div><div> bool VerifyOutput;</div><div> bool MergeFunctions;</div><div>+ bool LTO;</div></div><div><br></div><div>While in the context of clang it makes sense that "LTO" means "generate an object file appropriate for later LTO", but in the context of LLVM, it might be taken to mean "we are doing LTO optimizations now". I think we probably want to name the boolean something more specific (PreLTO? CompileForLTO?). Later if we customize further we can split the codepath as is done for populateLTOPassManager.</div><div><br></div><div>+ if (!LTO) {<br></div></div><div>+ // Remove avail extern fns and globals definitions if we aren't</div><div>+ // doing an -flto compilation. For LTO we will preserve these</div><div><br></div><div>LLVM doesn't have an -flto flag, so I'd probably keep this generic to "compiling an object file for later LTO".</div><div><br></div><div>+ // so they are eligible for inlining at link-time. Note if they</div><div>+ // are unreferenced they will be removed by GlobalDCE below, so</div><div>+ // this only impacts referenced available externally globals.</div><div>+ // Eventually they will be suppressed during codegen, but eliminating</div><div>+ // here is a compile-time savings.</div><div><br></div><div>More than the compile time savings, it actually increases the power of GlobalDCE and can reduce object file size, so I'd mention that instead.</div><div><br></div><div>+ MPM.add(createElimAvailExternPass());</div><div>+ }</div><div><br></div><div><div><div>+ // Drop initializers of available externally global variables.</div><div>+ for (Module::global_iterator I = M.global_begin(), E = M.global_end();</div><div>+ I != E; ++I) {</div><div>+ if (!I->hasAvailableExternallyLinkage())</div><div>+ continue;</div><div>+ if (I->hasInitializer()) {</div><div>+ Constant *Init = I->getInitializer();</div><div>+ I->setInitializer(nullptr);</div><div>+ if (isSafeToDestroyConstant(Init))</div><div>+ Init->destroyConstant();</div><div>+ }</div><div>+ I->removeDeadConstantUsers();</div><div>+ NumVariables++;</div><div>+ }</div></div></div><div><br></div><div>Do you need to set the linkage of global variables to external? I noticed that Function::deleteBody() does this but setInitializer(nullptr) does not. Ditto for aliases.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 9:03 AM, Teresa Johnson <span dir="ltr"><<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Talked to Eric Fri and he suggested that this might be the first of<br>
several places where we want behavior of LTO compiles to diverge from<br>
normal -O2 compiles. So for now I have implemented this such that we<br>
pass down -flto to the -cc1 job, and that gets propagated as a code<br>
gen option and into the PassManagerBuilder. I have left the current<br>
logic translating -flto to the -emit-llvm-bc option, since IMO that is<br>
cleaner for controlling the output type. Let me know what you think of<br>
saving the overall LTO setting in CodeGenOpts/PassManagerBuilder, or<br>
if is it better to record just the individual behaviors we want (i.e.<br>
change the new field in CodeGenOpts/PassManagerBuilder from "LTO" to<br>
"ElimAvailExtern" or something like that).<br>
<br>
Patches attached. I modified an existing available external clang test<br>
to check for the new O2 (LTO vs not) behaviors.<br>
<br>
Thanks,<br>
Teresa<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Jun 4, 2015 at 9:20 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
><br>
><br>
> On Thu, Jun 4, 2015 at 8:17 PM Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>> wrote:<br>
>><br>
>> On Thu, Jun 4, 2015 at 5:33 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
>> > On Thu, Jun 4, 2015 at 5:17 PM, Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Agreed. Although I assume you mean invoke the new pass under a<br>
>> >> ThinLTO-only option so that avail extern are not dropped in the<br>
>> >> compile pass before the LTO link?<br>
>> ><br>
>> ><br>
>> > No, this pass would actually be an improvement to the standard -O2<br>
>> > pipeline.<br>
>> > The special case is the regular LTO pass pipeline, which wants to run<br>
>> > GlobalDCE but doesn't want to drop available_externally function<br>
>> > definitions<br>
>> > until after linker-stage inlining.<br>
>><br>
>> Ok. It looks to me like the LTO compile step with -O2 -flto -c uses<br>
>> this same -O2 optimization pipeline as without LTO. Clang communicates<br>
>> the fact that we are doing an LTO compile to llvm via the<br>
>> -emit-llvm-bc flag, which just tells it to emit bitcode and exit<br>
>> before codegen. So I would either need to key off of that or setup a<br>
>> new flag to indicate to llvm that we are doing an LTO -c compile. Or<br>
>> is there some other way that I am missing?<br>
>><br>
>> Incidentally, we'll also want to skip this new pass and keep any<br>
>> referenced avail extern functions in the ThinLTO -c compile step for<br>
>> the same reasons (and there are no imported functions yet at that<br>
>> point).<br>
>><br>
><br>
> Ultimately for any planned LTO build we're going to want to do a different<br>
> pass pipeline, it's probably worth considering what should be done before<br>
> and during LTO.<br>
><br>
> -eric<br>
><br>
>><br>
>> Teresa<br>
>><br>
>> ><br>
>> > Consider this test case:<br>
>> ><br>
>> > declare void @blah()<br>
>> > define i32 @main() {<br>
>> > call void @foo()<br>
>> > ret i32 0<br>
>> > }<br>
>> > define available_externally void @foo() noinline {<br>
>> > call void @bar()<br>
>> > ret void<br>
>> > }<br>
>> > define linkonce_odr void @bar() noinline {<br>
>> > call void @blah()<br>
>> > ret void<br>
>> > }<br>
>> ><br>
>> > If I run opt -O2 on this and feed it to llc, it actually generates code<br>
>> > for<br>
>> > bar, even though there are no actual references to bar in the final<br>
>> > code:<br>
>> ><br>
>> > main: # @main<br>
>> > pushq %rax<br>
>> > callq foo<br>
>> > xorl %eax, %eax<br>
>> > popq %rdx<br>
>> > retq<br>
>> ><br>
>> > bar: # @bar<br>
>> > jmp blah # TAILCALL<br>
>> ><br>
>> > This corner case happens to come up organically with dllimported<br>
>> > classes,<br>
>> > which is why I happened to think of it. :) I'm happy with a flag to<br>
>> > globaldce for LTO and the original patch, honestly.<br>
>><br>
>><br>
>><br>
>> --<br>
>> Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com">tejohnson@google.com</a> | <a href="tel:408-460-2413" value="+14084602413">408-460-2413</a><br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br>
<br>
<br>
--<br>
Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com">tejohnson@google.com</a> | <a href="tel:408-460-2413" value="+14084602413">408-460-2413</a><br>
</div></div></blockquote></div><br></div>