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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

This is a nice cleanup and less invasive than I feared. I am concerned as noted below about losing all variable definition importing. Will try to apply this and do a performance comparison with spec to see if there are currently any effects.


================
Comment at: lib/Linker/LinkModules.cpp:405
@@ -425,3 +404,3 @@
 
   if (isPerformingImport() && !doImportAsDefinition(&GV))
     return false;
----------------
This seems like it should be redundant/combined with the condition added to the below if statement. Can't the below if statement just be "if (!isPerformingImport() && !DGV && ...)"? I.e. not need to check GlobalsToImport again since it was already checked in doImportAsDefinition, and if we reach the below if statement when isPerformingImport() then we do want this symbol? 

Alternatively, you could combine this and the below if into one block like:

if (isPerformingImport()) {
   if (!doImportAsDefinition(&GV))
       return false;
} else if (!DGV && ...)
   return false;


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:279
@@ +278,3 @@
+
+    Function *F = dyn_cast<Function>(SGV);
+    if (!F && isa<GlobalAlias>(SGV)) {
----------------
Moving this is unrelated to the rest of the patch?

================
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");
----------------
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.

================
Comment at: test/Transforms/FunctionImport/Inputs/funcimport.ll:23
@@ -21,1 +22,3 @@
 
+define void @globalfunc3() #0 {
+entry:
----------------
Why was globalfunc3 and staticvar2 added? Don't see anything checking them in the test.

================
Comment at: tools/llvm-link/llvm-link.cpp:213
@@ +212,3 @@
+      // Linkage Promotion and renaming
+      FunctionImportGlobalProcessing ThinLTOProcessing(*M, *Index,
+                                                       &GlobalsToImport);
----------------
Extend and invoke llvm::renameModuleForThinLTO instead. Ditto for similar block added to FunctionImport.cpp.

================
Comment at: tools/llvm-link/llvm-link.cpp:287
@@ +286,3 @@
+      // Promotion
+      FunctionImportGlobalProcessing ThinLTOProcessing(*M, *Index);
+      if (ThinLTOProcessing.run())
----------------
Use llvm::renameModuleForThinLTO instead


http://reviews.llvm.org/D18122