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

[David Li via llvm-commits]

davidxl added inline comments.

================
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,
----------------
It does symbol resolution -- so may be called :  resolveGlobalTo ?

================
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);
----------------
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' ?

================
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;
----------------
is SF->getComdat() call needed?

================
Comment at: lib/Linker/LinkModules.cpp:717
@@ +716,3 @@
+  const Comdat *C = ImportFunction->getComdat();
+  if (C && SGV->getComdat() == C)
+    return false;
----------------
Can the logic for variable be folded into doImportFunction method (with name change)?

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

================
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
----------------
We do lazy linking aggressively?

================
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)
----------------
Reason for this change ?

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

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


http://reviews.llvm.org/D14623