<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 12, 2015 at 2:02 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Nov 12, 2015 at 1:45 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Thu, Nov 12, 2015 at 1:21 PM, Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>><br>
> wrote:<br>
>><br>
>> On Thu, Nov 12, 2015 at 12:56 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> wrote:<br>
>> ><br>
>> ><br>
>> > On Thu, Nov 12, 2015 at 11:41 AM, Teresa Johnson via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> tejohnson created this revision.<br>
>> >> tejohnson added reviewers: rafael, dexonsmith.<br>
>> >> tejohnson added subscribers: llvm-commits, davidxl, joker.eph.<br>
>> >><br>
>> >> A number of comdat related fixes, cleanup and test cases.<br>
>> >><br>
>> >> I made a number of changes to get importing working when there are<br>
>> >> comdats, with lots of new test cases involving comdats of various<br>
>> >> selection types, linkage types, with and without aliases.<br>
>> >><br>
>> >> The changes ensure we link in the full comdat group containing the<br>
>> >> function indicated for importing. Previously I intended to require<br>
>> >> that the importer specify the additional comdat group members for<br>
>> >> importing on successive importing requests. However, in order to get<br>
>> >> the<br>
>> >> comdat selection type correct it is significantly simpler to import the<br>
>> >> full comdat group in a single pass.<br>
>> ><br>
>> ><br>
>> > I thought all imported definitions were marked available_externally - in<br>
>> > which case I'm not sure why it would be necessary to bring in the whole<br>
>> > comdat group, since the comdat would never be emitted here.<br>
>><br>
>> Not always, e.g. comdat containing linkonce values.<br>
>><br>
>> Even in the case of those brought in available_externally, my concern<br>
>> is when the comdat group also exists in the dest module (the one we<br>
>> are importing into), but we want to select the one from the source<br>
>> module (containing the imported function) because of the comdat<br>
>> selection type. I am concerned about importing part of the comdat<br>
>> group then eliminating it, leaving an incomplete group. Presumably the<br>
>> linker will select the complete group from the right .o file in the<br>
>> final link, but it seems wonky to me.<br>
><br>
><br>
> Sorry, I'm not quite following. My familiarity with comdats is perhaps<br>
> limited - but mostly for linkonce situations "we put this in multiple object<br>
> files, but we really only need one copy of the group in the final<br>
> executable".<br>
><br>
> I'm imagining an intermodule optimization where we promote the group in one<br>
> file to "always emitted (not just only emitted if called/used - because this<br>
> version will be relied upon by other modules now) but can still be<br>
> deduplicated by the linker" (maybe we don't have a linkage type for that?<br>
> Would weak suffice? From my reading of the LangRef it sounds like exactly<br>
> the right thing) and then marking them in all the other modules we see as<br>
> available_externally.<br>
><br>
> In this way, the definition would be available for optimization, but we<br>
> would avoid code generating duplicate entities only to have the linker<br>
> deduplicate them later. (but, since we don't have whole program information<br>
> - we'd still be able to deduplicate this single ThinLTO definition with any<br>
> non-ThinLTO definitions at link time)<br>
<br>
</div></div>ThinLTO isn't whole program, and what you are describing would need<br>
whole program analysis to determine which module will emit the comdat<br>
into the object file and guarantee that it will be emitted somewhere<br>
so that the other modules can drop it even if referencing the comdat<br>
members. </blockquote><div><br></div><div>Not sure I quite follow - why would this require whole program knowledge?<br><br>say you've got 3 files, two being ThinLTO'd, one is being compiled as normal and then all 3 objects are linked together<br><br>The all start off with a comdat group of linkonce_odr entities (one fo the ThinLTO'd ones might've got this naturally or by the ThinLTO process). One becomes weak_odr, one becomes available_externally, the other remains linkonce_odr.<br><br>When linked together we still end up with a definition.<br><br>Unless weak_odr + linkonce_odr doesn't collapse safely? (if not, what's the point of weak rather than external?)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Weak linkage does what you mention (keep it even if<br>
unreferenced, deduplicate in link), but like I said, ThinLTO doesn't<br>
have the visibility to make those kind of changes.<br>
<div><div class="h5"><br>
><br>
>><br>
>><br>
>> ><br>
>> > I guess I'm missing something - is there a pointer to when/why this is<br>
>> > necessary?<br>
>><br>
>> See the ThinLTO symbol linkage RFC here:<br>
>> <a href="https://groups.google.com/d/msg/llvm-dev/-6H4GLv2BJw/77DBR-VjuHkJ" rel="noreferrer" target="_blank">https://groups.google.com/d/msg/llvm-dev/-6H4GLv2BJw/77DBR-VjuHkJ</a>.<br>
>> There is a summary table at the end, but there are a lot of details.<br>
>> It has changed a little bit based on Duncan's feedback and other<br>
>> things found during implementation, but it is mostly accurate. See<br>
>> also ModuleLinker::getLinkage() in LinkModules.cpp for the implemented<br>
>> mapping.<br>
><br>
><br>
> Ah, thanks for the ref.<br>
><br>
> "WeakODRLinkage symbols cannot be marked AvailableExternallyLinkage, because<br>
> if the def is later dropped (by the EliminateAvailableExternally pass), the<br>
> new decl is marked ExternalLinkage. For these weak symbols, however, the<br>
> correct linkage for the decl is actually ExternalWeakLinkage, so that they<br>
> get treated appropriately by the linker."<br>
><br>
> That helps me understand some of it...<br>
><br>
> *reads further*<br>
><br>
> It's not called out, but I imagine there's the same problem with<br>
> LinkonceODRLinkage? We can't take a linkonce_odr function, change it to<br>
> weak_odr, import it into the other module as available_externally? Because<br>
> once we drop the definition, we lose the "weak" attribute and would produce<br>
> an incorrect reference?<br>
><br>
> That seems unfortunate - it's mentioned in the weak_odr case that weak<br>
> symbols are rare - linkonce_odr functions are probably /really/ common and<br>
> might benefit from this. But it's purely a linker/object size optimization,<br>
> so I could imagine it would be a valid optimization to defer until later?<br>
<br>
</div></div>WeakODR is one thing that changed since the proposal was sent (see<br>
Duncan't review and my response). They are now imported as<br>
available_externally defs (see comments in ModuleLinker::getLinkage<br>
for details).<br>
<br>
LinkOnceODR imported GVs stay LinkOnceODR. The difference between<br>
WeakODR and LinkOnceODR is because the latter is discardable if<br>
unused, so there is no guarantee that it will be emitted into the<br>
object file in its original module (e.g. if all uses inlined). WeakODR<br>
is not discardable if unused, so is guaranteed to be available.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> ><br>
>> > (& in fact, it might be useful to have this pass make one instance of a<br>
>> > comdat group into an external definition (I guess that's probably not<br>
>> > possible unless you have true whole program knowledge - maybe there's<br>
>> > another linkage type which would produce the comdat group even if all<br>
>> > the<br>
>> > calls are optimized away) - and mark all the other instances<br>
>> > available_externally? even potentially stripping the comdat groups from<br>
>> > modules where the call sites are really cold)<br>
>><br>
>> I think this is not possible without whole program analysis as you<br>
>> mentioned.<br>
>><br>
>> Thanks,<br>
>> Teresa<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> Additionally, if the module linker selects the source version of a<br>
>> >> comdat not containing the imported function, if it is referenced<br>
>> >> by an imported function we also import all its comdat group member<br>
>> >> definitions (via the lazy linker) in the same importing pass.<br>
>> >> In order to support this, we are now much more aggressive about lazy<br>
>> >> linking global values during importing, which only need to be<br>
>> >> linked in (as either declarations or definitions) if referenced by an<br>
>> >> imported function.<br>
>> >><br>
>> >> Also, because we may now import additional function bodies lazily, it<br>
>> >> is<br>
>> >> no longer possible to determine up front whether an alias's aliasee is<br>
>> >> going to be imported as a definition or not when we copy its prototype.<br>
>> >> Therefore, we now convert aliases into declarations after all lazy<br>
>> >> linking is complete, if the aliasee definition was not imported.<br>
>> >><br>
>> >> Finally, since comdat members may not be declarations (including<br>
>> >> available_externally), after linking is complete those that were<br>
>> >> imported as linker declarations will be removed from comdats.<br>
>> >><br>
>> >> <a href="http://reviews.llvm.org/D14623" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14623</a><br>
>> >><br>
>> >> Files:<br>
>> >> lib/Linker/LinkModules.cpp<br>
>> >> test/Linker/Inputs/funcimport_comdat.ll<br>
>> >> test/Linker/Inputs/funcimport_comdat2.ll<br>
>> >> test/Linker/Inputs/funcimport_comdat3.ll<br>
>> >> test/Linker/funcimport.ll<br>
>> >> test/Linker/funcimport_comdat.ll<br>
>> >> test/Linker/funcimport_comdat2.ll<br>
>> >> test/Linker/funcimport_comdat3.ll<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</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>
><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>