<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jun 8, 2015 at 11:46 AM Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jun 8, 2015 at 11:33 AM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>
> The clang patch lgtm, and I had some comments on the LLVM patch. Duncan, do<br>
> you want to say more there?<br>
><br>
> --- include/llvm/Transforms/IPO/PassManagerBuilder.h (revision 237590)<br>
> +++ include/llvm/Transforms/IPO/PassManagerBuilder.h (working copy)<br>
> @@ -121,6 +121,7 @@ class PassManagerBuilder {<br>
> bool VerifyInput;<br>
> bool VerifyOutput;<br>
> bool MergeFunctions;<br>
> + bool LTO;<br>
><br>
> While in the context of clang it makes sense that "LTO" means "generate an<br>
> object file appropriate for later LTO", but in the context of LLVM, it might<br>
> be taken to mean "we are doing LTO optimizations now". I think we probably<br>
> want to name the boolean something more specific (PreLTO? CompileForLTO?).<br>
> Later if we customize further we can split the codepath as is done for<br>
> populateLTOPassManager.<br>
<br>
Yeah, I was concerned that the name might be confusing in this<br>
context, so if we opt to go with a single llvm option to mean -flto I<br>
like the idea of something like CompileForLTO (or maybe<br>
PrepareForLTO?).<br>
<br></blockquote><div><br></div></div><div dir="ltr"><div class="gmail_quote"><div>I'd rather not have it be an llvm option at all. Just construct a different set of passes...</div><div><br></div><div>This would also solve the problem of needing multiple sets of options to be passed to the builder. It'd be a bit of a change (i.e. having clang do the pass setup), but I think it'd be worth it to have clang be able to explicitly say which passes it wants.</div><div><br></div><div>It's also a major change and would probably need to be discussed more widely so for now I guess this is sorta ok.</div></div></div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> More than the compile time savings, it actually increases the power of<br>
> GlobalDCE and can reduce object file size, so I'd mention that instead.<br>
<br>
Ok, right in your case that is the benefit. Will update the comment.<br>
<br></blockquote><div><br></div><div>In yours too, just by keeping the amount of IR smaller :)</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> + MPM.add(createElimAvailExternPass());<br>
> + }<br>
><br>
> + // Drop initializers of available externally global variables.<br>
> + for (Module::global_iterator I = M.global_begin(), E = M.global_end();<br>
> + I != E; ++I) {<br>
> + if (!I->hasAvailableExternallyLinkage())<br>
> + continue;<br>
> + if (I->hasInitializer()) {<br>
> + Constant *Init = I->getInitializer();<br>
> + I->setInitializer(nullptr);<br>
> + if (isSafeToDestroyConstant(Init))<br>
> + Init->destroyConstant();<br>
> + }<br>
> + I->removeDeadConstantUsers();<br>
> + NumVariables++;<br>
> + }<br>
><br>
> Do you need to set the linkage of global variables to external? I noticed<br>
> that Function::deleteBody() does this but setInitializer(nullptr) does not.<br>
> Ditto for aliases.<br>
<br>
Hmm, I am not sure - I basically am doing the same things GlobalDCE<br>
does (aside from erasing them from the function/variable/alias list).<br>
I guess it didn't matter in the GlobalDCE case since they were being<br>
completely erased rather than leaving a decl behind. Will make that<br>
change and retest.<br>
<br>
Thanks!<br>
Teresa<br>
<br>
><br>
> On Mon, Jun 8, 2015 at 9:03 AM, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br>
>><br>
>> 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>
>><br>
>> On Thu, Jun 4, 2015 at 9:20 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>><br>
>> wrote:<br>
>> ><br>
>> ><br>
>> > On Thu, Jun 4, 2015 at 8:17 PM Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Thu, Jun 4, 2015 at 5:33 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>
>> >> > On Thu, Jun 4, 2015 at 5:17 PM, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">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<br>
>> > different<br>
>> > pass pipeline, it's probably worth considering what should be done<br>
>> > 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<br>
>> >> > 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" target="_blank">tejohnson@google.com</a> |<br>
>> >> 408-460-2413<br>
>> >> _______________________________________________<br>
>> >> LLVM Developers mailing list<br>
>> >> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">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" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
><br>
><br>
<br>
<br>
<br>
--<br>
Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
</blockquote></div></div></div>