[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())
----------------
tejohnson wrote:
> 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?
yes, In theory, the combined index generator can do comdat de-duplication before writing out the combined summary -- but since that step is mandatory, handling duplication is still required here.

================
Comment at: lib/Linker/LinkModules.cpp:441
@@ +440,3 @@
+  /// imported as declarations instead of definitions.
+  Function *ImportFunction;
+
----------------
tejohnson wrote:
> 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.
ok.

================
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,
----------------
tejohnson wrote:
> 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.
ok.

================
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.
----------------
tejohnson wrote:
> 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.
sounds like this can be done in place (making dstM the same as srcM), but as it is existing code, ok.

================
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;
----------------
tejohnson wrote:
> 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).
When promoting Constant Global, make sure the initializer is made available so that const prop can still happen.

================
Comment at: lib/Linker/LinkModules.cpp:688
@@ +687,3 @@
+        ImportIndex->getModuleId(SGV->getParent()->getModuleIdentifier()));
+  return SGV->getName();
+}
----------------
tejohnson wrote:
> 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.
ok.

================
Comment at: lib/Linker/LinkModules.cpp:745
@@ +744,3 @@
+      if (doImportAsDefinition(SGV))
+        return GlobalValue::AvailableExternallyLinkage;
+      else
----------------
tejohnson wrote:
> 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.
ok. I got it confused with LinkOnceODR.


http://reviews.llvm.org/D13515