[PATCH] D18343: ThinLTO: use the callgraph from the combined index to drive the FunctionImporter

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Thanks, good catch here. Updated!


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:157
@@ +156,3 @@
+    // as exported from outside as well.
+    // FIXME: we don't import any globals right now. We may want to import
+    // constants?
----------------
tejohnson wrote:
> True that we don't import globals, but we do export references to them if they are referenced from a function that is imported elsewhere. So presumably those references should be added to the export list as they may need to be promoted/renamed.
Oh, good point, my previous answer was totally wrong, I thought we were looking at the "import" list here. When I wrote the original code I intended `Summary->edge()` to iterate over both calls() and refs(). Fixed!

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:324
@@ +323,3 @@
+        // When the alias is pointing to a linkonceodr, don't import
+        // FIXME: why?
+        //          continue;
----------------
tejohnson wrote:
> Oh, I know why. We can't have an alias to an available_externally def, since that is actually a declaration for the linker, and most imported defs are given available_externally linkage (except linkonce which stays the same linkage). This is the same logic as in FunctionImportGlobalProcessing::doImportAsDefinition(). I think there might have been more comments around that at one point but they gotten lost when the code moved around.
Updated comment


http://reviews.llvm.org/D18343