[PATCH] D18343: ThinLTO: use the callgraph from the combined index to drive the FunctionImporter
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 21:48:50 PDT 2016
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?
> 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;
> 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.
More information about the llvm-commits