<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 15, 2015 at 1:15 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Fri, May 15, 2015 at 10:04 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, May 15, 2015 at 9:53 AM, Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>><br>
> wrote:<br>
>><br>
>> On Fri, May 15, 2015 at 9:20 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Fri, May 15, 2015 at 9:18 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >>><br>
>> >>><br>
>> >>> >> - Marking of imported functions (for use in ThinLTO-specific symbol<br>
>> >>> >> linking and global DCE, for example).<br>
>> >>> ><br>
>> >>> > Marking how?  Do you mean giving them internal linkage, or something<br>
>> >>> > else?<br>
>> >>><br>
>> >>> Mentioned just after this: either an in-memory flag on the Function<br>
>> >>> class, or potentially in the IR. For the prototype I just had a flag<br>
>> >>> on the Function class.<br>
>> >><br>
>> >><br>
>> >> Would this be anything other than "available externally" linkage?<br>
>> >> (either<br>
>> >> this module is responsible for emitting the definition of this function<br>
>> >> for<br>
>> >> other modules to call, or it's not - if it's not, either this module<br>
>> >> depends<br>
>> >> on another module to provide a definition (this would be "available<br>
>> >> externally") or it doesn't (this would be "internal" linkage")) - I<br>
>> >> think...<br>
>> >> though I'm sort of jumping into the middle of some of this & might've<br>
>> >> missed<br>
>> >> some context, sorry.<br>
>> ><br>
>> ><br>
>> > & I see your follow up further down in that thread:<br>
>> ><br>
>> > "There were only a couple minor tweaks required here (under the flag I<br>
>> > added to the Function indicating that this was imported). Only<br>
>> > promoted statics are remarked available_externally. For a<br>
>> > non-discardable symbol that was imported, we can discard here since we<br>
>> > are done with inlining (it is non-discardable in its home module)."<br>
>> ><br>
>> > &, like Duncan, I'll wait for more details on that front. (may or may<br>
>> > not be<br>
>> > useful to split some of these subthreads into separate email threads to<br>
>> > keep<br>
>> > discussion clear - but I'm not sure)<br>
>><br>
>> I just went back and looked at my prototype and I had remembered this<br>
>> wrong. An imported function is always marked<br>
>> AvailableExternallyLinkage, unless it has link once linkage.<br>
>><br>
>> As far as using that to indicate that it is an aux function, I was<br>
>> concerned about overloading the meaning of that linkage type. See the<br>
>> next para for an example where I am unsure about doing this...<br>
>><br>
>> Looking back through my GlobalDCE changes, it looks like one of the<br>
>> places I had changed (where we mark defined globals in runOnModule)<br>
>> already has a guard for !hasAvailableExternallyLinkage and<br>
>> !isDiscardableIfUnused, so my additional guard against marking<br>
>> imported functions is unnecessary. But the other place I had to change<br>
>> was in GlobalIsNeeded where it walks through the function and<br>
>> recursively marks any referenced global as needed. Here there was no<br>
>> guard against marking a global that is available externally as needed<br>
>> if it is referenced. I had added a check here to not mark imported<br>
>> functions as needed on reference unless they were discardable (i.e.<br>
>> link once). Is this a bug - should this have a guard against marking<br>
>> available externally function refs as needed?<br>
><br>
><br>
> Duncan's probably got a better idea of what the right answer is here. I<br>
> suspect "yes".<br>
><br>
> The trick with available_externally is to ensure we keep these around for<br>
> long enough that their definitions are useful (for inlining, constant prop,<br>
> all that good stuff) but remove them before we actually do too much work on<br>
> them, or at least before we emit them into the final object file.<br>
<br>
</div></div>Yep, and that is exactly how long I want my imported functions to<br>
stick around. Currently I have the ThinLTO import pass added at the<br>
start of addLTOOptimizationPasses, just after the AA passes. So the<br>
imported functions stick around through global opt, constant merge,<br>
combining, and inlining. The call to GlobalDCE is just after inlining<br>
and a second globalopt.<br>
<span class=""><br>
><br>
> I imagine if GlobalDCE isn't removing available_externally functions it's<br>
> because they're still useful in the optimization pipeline and something<br>
> further down the pipe removes them (because they do, ultimately, get<br>
> removed).<br>
<br>
</span>That would be good to know, since if they are useful later on in the<br>
pipe then presumably my imported functions could be as well. But I<br>
wonder who is removing them since GlobalDCE isn't (if there are refs)?<br></blockquote><div><br>Yep - seems no one removes them (well, probably if there are no calls to the function at all they get removed as usual (eg: if all the calls are inlined we probably drop the function as we would for a linkonce_odr function)). If they're still around come codegen time, we just skip them:<br><br>lib/CodeGen/MachineFunctionPass.cpp, MachineFunctionPass::runOnFunction:<br><br><div><font face="monospace, monospace">33| bool MachineFunctionPass::runOnFunction(Function &F) {</font></div><div><font face="monospace, monospace">34|   // Do not codegen any 'available_externally' functions at all, they have</font></div><div><font face="monospace, monospace">35|   // definitions outside the translation unit.</font></div><div><font face="monospace, monospace">36|   if (F.hasAvailableExternallyLinkage())</font></div><div><font face="monospace, monospace">37|     return false;</font></div><div><font face="monospace, monospace">38|</font></div><div><font face="monospace, monospace">39|   MachineFunction &MF = getAnalysis<MachineFunctionAnalysis>().getMF();</font></div><div><font face="monospace, monospace">40|   return runOnMachineFunction(MF);</font></div><div><font face="monospace, monospace">41| }</font></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> Any idea what problematic behavior you were seeing that was<br>
> addressed by changing DCE to remove these functions was? I imagine whatever<br>
> the bad behavior was, it could be demonstrated without ThinLTO and with a<br>
> normal module with an available_externally function & would be worth<br>
> discussing what the right behavior is in that context, regardless of<br>
> ThinLTO.<br>
<br>
</span>So I never changed GlobalDCE to eliminate available externally<br>
functions in general, because I came at this with the view that I<br>
shouldn't rely on just the linkage info. But I can certainly try that<br>
experiment. What isn't clear to me is what all uses the available<br>
externally linkage type currently - do you happen to know?<br>
<br>
Thanks,<br>
Teresa<br>
<div class=""><div class="h5"><br>
><br>
>><br>
>><br>
>> There was one other change to GlobalDCE that I had to make, where we<br>
>> erase the list of DeadFunctions from the module. In my case we may<br>
>> have a function body that was eliminated, but still have references to<br>
>> it (i.e. in the case I am talking about above). In that case it is now<br>
>> just a declaration and we don't erase it from the function list on the<br>
>> module. If the GlobalsIsNeeded code is changed to avoid marking<br>
>> available externally function refs as needed then this change would be<br>
>> needed for that case as well.<br>
>><br>
>> Thanks,<br>
>> Teresa<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >><br>
>> >> - David<br>
>> ><br>
>> ><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>
><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></div>