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

[Teresa Johnson via llvm-commits]

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)
       M.release();
----------------
I thought I removed the PreserveModules code from HEAD awhile back? How did it get merged back?


http://reviews.llvm.org/D18122