[PATCH] D13515: Support for ThinLTO function importing and symbol linking.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/IR/FunctionInfo.h:235
@@ +234,3 @@
+                                         getModuleId(M->getModuleIdentifier()));
+      for (const auto &FI : getFunctionInfoList(FuncName)) {
+        if (M->getModuleIdentifier() == FI->functionSummary()->modulePath())
----------------
comdat functions should have been merged at this point, right?

================
Comment at: lib/Linker/LinkModules.cpp:441
@@ +440,3 @@
+  /// imported as declarations instead of definitions.
+  Function *ImportFunction;
+
----------------
Do we want to support importing a list of functions in a batch mode?

================
Comment at: lib/Linker/LinkModules.cpp:450
@@ -430,6 +449,3 @@
 public:
-  ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,
-               DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags)
-      : DstM(dstM), SrcM(srcM), TypeMap(Set),
-        ValMaterializer(TypeMap, DstM, LazilyLinkGlobalValues),
-        DiagnosticHandler(DiagnosticHandler), Flags(Flags) {}
+ ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,
+              DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags,
----------------
Is it better to add another flag to indicate whether it is for primary module or imported modules.

================
Comment at: lib/Linker/LinkModules.cpp:466
@@ +465,3 @@
+   // then this is the primary module being compiled in a ThinLTO
+   // backend compilation, and we need to see if it has functions that
+   // may be exported to another backend compilation.
----------------
assert dstM == srcM?

================
Comment at: lib/Linker/LinkModules.cpp:573
@@ +572,3 @@
+  /// a local that is being promoted to global scope.
+  StringRef getNewName(const GlobalValue *SGV);
+
----------------
How about just getName or getLinkName? The name may or may not be new.

================
Comment at: lib/Linker/LinkModules.cpp:676
@@ +675,3 @@
+  // can simply use the clone created in this module.
+  if (GVar && GVar->isConstant()) return false;
+  return true;
----------------
Unless the address taken bit is set (to handle the case address is used in comparison).

================
Comment at: lib/Linker/LinkModules.cpp:688
@@ +687,3 @@
+        ImportIndex->getModuleId(SGV->getParent()->getModuleIdentifier()));
+  return SGV->getName();
+}
----------------
For non promoted constant local globals, the name also needs to be uniqued -- otherwise there may be conflicts in the assembly file (some constants are materialized using memory read).

================
Comment at: lib/Linker/LinkModules.cpp:719
@@ +718,3 @@
+      // to external if imported as a declaration.
+      if (isPerformingImport() && !doImportAsDefinition(SGV))
+        return GlobalValue::ExternalLinkage;
----------------
isPerformingImport() check is redundant here.

================
Comment at: lib/Linker/LinkModules.cpp:745
@@ +744,3 @@
+      if (doImportAsDefinition(SGV))
+        return GlobalValue::AvailableExternallyLinkage;
+      else
----------------
I think it should stay as WeakODRLinkage -- there is no guarantee that it will be emitted in other modules (if there is no reference).

================
Comment at: lib/Linker/LinkModules.cpp:771
@@ +770,3 @@
+    case GlobalValue::ExternalWeakLinkage:
+      // External weak doesn't apply to declarations, must be a definition.
+      assert(!doImportAsDefinition(SGV));
----------------
The comment seems to mean the opposite.


http://reviews.llvm.org/D13515