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

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 16 11:02:47 PDT 2015


tejohnson added a comment.

Thanks for the comments. Responses below.


================
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())
----------------
davidxl wrote:
> comdat functions should have been merged at this point, right?
Merged where? At the point we invoke this we are just getting a module ready to import from - we would not have merged comdats across modules. And the FunctionInfoList may contain multiple copies of a comdat, unless we decided in the stage-2 global indexing step to merge their entries in the combined index. Presumably due to the way this is being used to guard renaming and promotion of locals, we couldn't remove an entry for any comdat copy that accessed a local - but I doubt that could happen?

================
Comment at: lib/Linker/LinkModules.cpp:441
@@ +440,3 @@
+  /// imported as declarations instead of definitions.
+  Function *ImportFunction;
+
----------------
davidxl wrote:
> Do we want to support importing a list of functions in a batch mode?
Good question, and something I was pondering. For now I think we should support one at a time, and I can extend this to support importing a set in the future if it seems useful. Part of the issue is that until you import a function, you won't know what other function in the same module are called by it that you may now want to import. But presumably you may have multiple functions from the same source module called from the current primary module that you know you want to import, and batch mode could help there. But I would prefer to add that handling as a follow-on if it is necessary.

================
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,
----------------
davidxl wrote:
> Is it better to add another flag to indicate whether it is for primary module or imported modules.
That is implied by whether FuncToImport is null or not. A flag would be redundant.

================
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.
----------------
davidxl wrote:
> assert dstM == srcM?
That should never be the case - the dstM is the "combined" module being created by the link. Even if we don't import, the module being linked is not the same as the srcM.

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

================
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;
----------------
davidxl wrote:
> Unless the address taken bit is set (to handle the case address is used in comparison).
Good catch. And unfortunately for GVars there is no address taken bit or easy check for this. For functions, there is a hasAddressTaken function that walks the uses, but not for GVars. But that isn't useful here in the importing context in any case, since we haven't even parsed all of the functions where it may be address taken. It will have to be recorded in the function index in the front end. For now I will simply promote all locals (and leave a comment about what would need to be done to reduce this scope).

================
Comment at: lib/Linker/LinkModules.cpp:688
@@ +687,3 @@
+        ImportIndex->getModuleId(SGV->getParent()->getModuleIdentifier()));
+  return SGV->getName();
+}
----------------
davidxl wrote:
> 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).
Good point. Since I will now promote all locals, this is somewhat moot, but will resurface if we do something more narrow to identify the address taken locals that need promotion. I will change this to rename all locals when we are importing or exporting.

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

================
Comment at: lib/Linker/LinkModules.cpp:745
@@ +744,3 @@
+      if (doImportAsDefinition(SGV))
+        return GlobalValue::AvailableExternallyLinkage;
+      else
----------------
davidxl wrote:
> I think it should stay as WeakODRLinkage -- there is no guarantee that it will be emitted in other modules (if there is no reference).
WeakODR is not discardable, unlike LinkOnceODR. So it will be emitted in the original module.

================
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));
----------------
davidxl wrote:
> The comment seems to mean the opposite.
You're right, will fix the comment.


http://reviews.llvm.org/D13515





More information about the llvm-commits mailing list