[PATCH] D18122: Rework linkInModule(), making it oblivious to ThinLTO

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 16 04:18:18 PDT 2016

tejohnson added inline comments.

Comment at: lib/Transforms/IPO/FunctionImport.cpp:360
@@ -352,3 +359,3 @@
     if (TheLinker.linkInModule(std::move(SrcModule), Linker::Flags::None,
-                               &Index, &FunctionsToImport))
+                               &FunctionsToImport))
       report_fatal_error("Function Import: link error");
joker.eph wrote:
> tejohnson wrote:
> > Rename FunctionsToImport to GlobalsToImport here as well?
> > 
> > Although currently with this change it looks like we will never import a global variable definition since the importer is only adding functions to this list, right? I wonder if there is a performance loss. Will apply this and try with spec.
> Yes no import of globals yet, we operate only on functions.  It makes sense only for constant, right? I don't know if we have this information in the summary?
Ran spec with this patch applied, no significant performance delta, so this is not currently a big issue. 

Might have to augment the summary again to note if they are constant.

Comment at: lib/Transforms/IPO/FunctionImport.cpp:441
@@ -438,3 +440,3 @@
     // are potentially exported to other modules.
-    if (renameModuleForThinLTO(M, *Index)) {
+    if (renameModuleForThinLTO(M, *Index, nullptr)) {
       errs() << "Error renaming module\n";
Use a default on the new parameter of nullptr instead of passing it explicitly?

Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:227
@@ -236,2 +226,3 @@
+  FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
   return ThinLTOProcessing.run();
Shouldn't the getNewExportedValues be added to GlobalsToImport if non-null?

Comment at: tools/llvm-link/llvm-link.cpp:305
@@ -293,3 +304,3 @@
     // cross-module references.
     if (PreserveModules)
I thought I removed the PreserveModules code from HEAD awhile back? How did it get merged back?


More information about the llvm-commits mailing list