Page MenuHomePhabricator

[ThinLTO] Comdat importing fixes and related cleanup
AbandonedPublic

Authored by tejohnson on Nov 12 2015, 11:41 AM.

Details

Summary

A number of comdat related fixes, cleanup and test cases.

I made a number of changes to get importing working when there are
comdats, with lots of new test cases involving comdats of various
selection types, linkage types, with and without aliases.

The changes ensure we link in the full comdat group containing the
function indicated for importing. Previously I intended to require
that the importer specify the additional comdat group members for
importing on successive importing requests. However, in order to get the
comdat selection type correct it is significantly simpler to import the
full comdat group in a single pass.

Additionally, if the module linker selects the source version of a
comdat not containing the imported function, if it is referenced
by an imported function we also import all its comdat group member
definitions (via the lazy linker) in the same importing pass.
In order to support this, we are now much more aggressive about lazy
linking global values during importing, which only need to be
linked in (as either declarations or definitions) if referenced by an
imported function.

Also, because we may now import additional function bodies lazily, it is
no longer possible to determine up front whether an alias's aliasee is
going to be imported as a definition or not when we copy its prototype.
Therefore, we now convert aliases into declarations after all lazy
linking is complete, if the aliasee definition was not imported.

Finally, since comdat members may not be declarations (including
available_externally), after linking is complete those that were
imported as linker declarations will be removed from comdats.

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 40076.Nov 12 2015, 11:41 AM
tejohnson retitled this revision from to [ThinLTO] Comdat importing fixes and related cleanup.
tejohnson updated this object.
tejohnson added reviewers: rafael, dexonsmith.
tejohnson updated this revision to Diff 40103.Nov 12 2015, 7:15 PM
  • Add back a few test changes that got lost in the merge of r252932.

Ping.

Thanks,
Teresa

davidxl added inline comments.Nov 17 2015, 8:26 PM
lib/Linker/LinkModules.cpp
483

It does symbol resolution -- so may be called : resolveGlobalTo ?

572

Can you update the comment here? It is unclear what the function does with current comment.

Perhaps: Return true if \p F is either the same function as the one being imported or in same same comdat group as the imported function.

The name may better be 'needToImport' ?

700

is SF->getComdat() call needed?

721

Can the logic for variable be folded into doImportFunction method (with name change)?

944

why is the replacement needed?

1455

We do lazy linking aggressively?

1640

Reason for this change ?

1663

How can we be sure that the affected comdat does not have any remaining def in it?

1704

what is the difference beween DGA and NewGV? Are they both the global alias in DestModule?

Thanks for the comments. Will upload a new patch in a little while once I finish the changes and retesting.

lib/Linker/LinkModules.cpp
483

Note I haven't changed anything in this routine - just moved it up so that it is public not private and can be called from the value materializer. So I'd prefer not to change the name here.

572

Will change comment as suggested. I like the form of the name because it is consistent with the other methods below. I see your suggestion below about generalizing this to handle non-function GVs. I will change the name to "mustImport" and take a GlobalValue so that it can check for variables in the same comdat group as well.

700

Yes. The doImportFunction only looks for functions in the same comdat group as the requested import function. If we are going to import a symbol that is part of any comdat group it needs to be imported as a definition not declaration (see comment just above).

721

Good idea, will do (see details in my reply above).

944

Because we had a corresponding GV already in the dest module, and if we get here we have decided to override with a value from the source module (e.g. a comdat where the selection type is "largest" and the source module had a larger sized leading value that the dest module). Note the same thing is done for non-lazy-linked GVs at the end of ModuleLinker::linkGlobalValueProto. The difference that required adding this here is that before this patch we only lazy linked when there was no dest copy, but for function importing we will lazy link even when there is a dest copy.

1455

Yes, in the sense that before this patch we only lazy link when there is no copy of the GV already in the dest, but with function importing we can lazy link in many more cases and will now do so. I think the wording of my comment is funny. Probably I should change this first sentence to something like "In the case of ThinLTO function importing, we can lazy link most symbols, linking them in only if they are referenced by other imported functions."

1640

As I was writing up my response I realized this change is not only no longer needed but also not quite correct. I have reverted this change, and updated a test case to catch the problem it introduced.

1663

Since there is no pointer from Comdat to member, I can add some checking code under #ifndef NDEBUG that adds all the stripped comdats to a set and checks the GVs in DstM to see if any has a comdat in that set after the stripping is done.

1704

DGA is a GlobalAlias, which must be a definition. NewGV is a declaration that is either a GlobalVariable (created just above here), or a Function (created in the below case), cloned from the aliasee prototype.

tejohnson updated this revision to Diff 40521.Nov 18 2015, 8:56 AM
  • Address comments from davidxl.
davidxl added inline comments.Nov 18 2015, 9:35 AM
lib/Linker/LinkModules.cpp
944

A more general question -- why can't the attributes etc from Src GV be merged into Dst GV instead of creating a clone?

1704

This brings up the same question why the clone is needed in the first place..

tejohnson added inline comments.Nov 18 2015, 9:44 AM
lib/Linker/LinkModules.cpp
944

Probably more complicated than necessary when we create a NewGV that has a blend of attributes from the dest and source GV copies - we need to keep the dest copy around long enough to create the NewGV anyway. Note this is how the existing handling in linkGlobalValueProto works.

1704

See above answer. Also, because we import the declaration (prototype) before we can determine whether the definition needs to be linked in, it is much simpler to do this as a post-pass.

Ping.

Thanks, Teresa

rafael added inline comments.Nov 30 2015, 7:36 AM
lib/Linker/LinkModules.cpp
558

We cannot convert aliases to declarations.

tejohnson added inline comments.Nov 30 2015, 8:39 AM
lib/Linker/LinkModules.cpp
558

Why? Note that after the conversion it isn't an alias, just a declaration to an externally defined symbol. This is needed to import anything that references an alias (especially now with your change in r254170).

I.e. if we import function foo() and it calls f() which is actually an alias to function f2(). If we don't import f2 (or with r254170 if f2() is not linkonce), then f() cannot be an alias in the destination (importing) module. So we convert the imported f() alias into a declaration. The linker will resolve this to the f() alias in the importing module eventually. It shouldn't be any different than having a module reference an alias defined elsewhere originally.

Also, note this is already done (in copyGlobalAliasProto), I am just moving it to a post-pass here.

tejohnson abandoned this revision.Jan 12 2016, 11:18 AM

Most of this is now obsolete with Rafael's changes to split the linker. I still need the portion that removes imported available externally defs from comdats, but even that is simplified. I will create a separate patch for that part and the associated test and send for review shortly.