[PATCH] D14623: [ThinLTO] Comdat importing fixes and related cleanup

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

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


================
Comment at: lib/Linker/LinkModules.cpp:483
@@ +482,3 @@
+  /// destination module that is being linked to, if any.
+  GlobalValue *getLinkedToGlobal(const GlobalValue *SrcGV) {
+    // If the source has no name it can't link.  If it has local linkage,
----------------
davidxl wrote:
> It does symbol resolution -- so may be called :  resolveGlobalTo ?
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.

================
Comment at: lib/Linker/LinkModules.cpp:572
@@ -566,1 +571,3 @@
 
+  /// Is this the function requested for import or in the same comdat group?
+  bool doImportFunction(const Function *F);
----------------
davidxl wrote:
> 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' ?
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.

================
Comment at: lib/Linker/LinkModules.cpp:696
@@ -671,3 +695,3 @@
   auto *SF = dyn_cast<Function>(SGV);
-  if (SF && SF == ImportFunction)
+  if (SF && (doImportFunction(SF) || SF->getComdat()))
     return true;
----------------
davidxl wrote:
> is SF->getComdat() call needed?
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).

================
Comment at: lib/Linker/LinkModules.cpp:717
@@ +716,3 @@
+  const Comdat *C = ImportFunction->getComdat();
+  if (C && SGV->getComdat() == C)
+    return false;
----------------
davidxl wrote:
> Can the logic for variable be folded into doImportFunction method (with name change)?
Good idea, will do (see details in my reply above).

================
Comment at: lib/Linker/LinkModules.cpp:948
@@ +947,3 @@
+  // ValueMap for SGV.
+  if (DGV && NewGV != DGV) {
+    DGV->replaceAllUsesWith(ConstantExpr::getBitCast(NewGV, DGV->getType()));
----------------
davidxl wrote:
> why is the replacement needed?
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.

================
Comment at: lib/Linker/LinkModules.cpp:1459
@@ -1428,1 +1458,3 @@
 
+    // In the case of ThinLTO function importing, we are aggressive about lazy
+    // linking, i.e. only when referenced by imported functions. The exception
----------------
davidxl wrote:
> We do lazy linking aggressively?
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."

================
Comment at: lib/Linker/LinkModules.cpp:1644
@@ -1601,2 +1643,3 @@
     for (auto *SGV : ComdatMembers[SC]) {
-      if (ValueMap[SGV])
+      const GlobalValue *DGV = DstM->getNamedValue(getName(SGV));
+      if (DGV)
----------------
davidxl wrote:
> Reason for this change ?
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.

================
Comment at: lib/Linker/LinkModules.cpp:1668
@@ +1667,3 @@
+/// contain declarations.
+void ModuleLinker::stripDeclsFromComdats() {
+  for (GlobalVariable &SGV : SrcM->globals()) {
----------------
davidxl wrote:
> How can we be sure that the affected comdat does not have any remaining def in it?
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.

================
Comment at: lib/Linker/LinkModules.cpp:1709
@@ +1708,3 @@
+    if (auto *GVar = dyn_cast<GlobalVariable>(GVal))
+      NewGV = copyGlobalVariableProto(TypeMap, GVar);
+    else {
----------------
davidxl wrote:
> what is the difference beween DGA and NewGV? Are they both the global alias in DestModule?
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.


http://reviews.llvm.org/D14623