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

[Mehdi AMINI via llvm-commits]

joker.eph marked an inline comment as done.

================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:50
@@ +49,3 @@
+/// \p ImportLists will be populate with an enty for every Module. This entry is
+/// itself a map with an entry for each Module to import from, that contains a
+/// map of imported function (GUID) and their imported Threshold.
----------------
tejohnson wrote:
> Outer StringMap has an entry for every Module we are importing to, with an entry for each Module we are importing from, right? Maybe make that clearer by changing the first sentence above to "ImportLists will be populated with an entry for every Module we are importing into."
> 
> Might be clearer if you use typedefs for each map.
Typedef solve it all :)

I'll probably commit a subsequent cleanup to make GUID a typedef as well.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:70
@@ +69,3 @@
+/// FIXME: select "best" instead of first that fits. But what is "best"? The
+/// smallest? The one with the least edges? A mix?
+static const FunctionSummary *
----------------
tejohnson wrote:
> davidxl wrote:
> > tejohnson wrote:
> > > - Prioritize copies from a module already being imported from (reduces # source modules parsed/linked)
> > > - Prioritize any that has profile data, although we don't have that info here (not sure how COMDAT profile matching works on llvm, I know on gcc only the selected or any inlined/selected copies would get profile data, may be different on llvm due to single profile database?)
> > In LLVM, comdat function's profile counters will also be put into comdat, so there should be no need to differentiate based on profile availability.
> Good news!
Updated the comment.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:75
@@ +74,3 @@
+      CalleeInfoList, [&](const std::unique_ptr<GlobalValueInfo> &GlobInfo) {
+        auto *Summary = dyn_cast_or_null<FunctionSummary>(GlobInfo->summary());
+        if (!Summary)
----------------
tejohnson wrote:
> I think this can be an assert? We should have a summary for every GlobalValueInfo in the combined map, and presumably it should be a FunctionSummary type since we got here via the GUID of a call graph edge.
I think this was originally an assert and I figured later it was a legit case. Can't remember, will turn back into an assert for now.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:95
@@ +94,3 @@
+/// null if there's no match.
+static const FunctionSummary *selectCallee(uint64_t GUID, unsigned Threshold,
+                                           const ModuleSummaryIndex &Index) {
----------------
davidxl wrote:
> joker.eph wrote:
> > davidxl wrote:
> > > joker.eph wrote:
> > > > davidxl wrote:
> > > > > Why not passing a reference of the Edge to 'selectCallee'. We will need to look at profile data in the future.
> > > > I think the profile info will not impact *which* callee you select right? What do you expect to do with this information at this level?
> > > It will impact importing strategy. For instance increased threshold for hot callsites and reduced threshold for extreme cold ones.  The summary Edge can also contain other nice bits  (such as has constant argument etc) which will help importing more precisely.
> > I agree, it is not clear to me that exposing the "Edge" here is the right abstraction (one could modifying the Threshold at call site to include a PGO bias for instance, even if it is probably not the right thing either).
> > 
> > I think that I can safely leave this for the larger refactoring work that will follow when we will rework the cost model for importing. 
> Sounds fine to me.
Great, thanks David!

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:307
@@ +306,3 @@
+        const GlobalObject *GO = GV.getBaseObject();
+        if (!GO->hasLinkOnceODRLinkage())
+          continue;
----------------
tejohnson wrote:
> Add comment about why this is done?
I have no idea, I remember copying it from what I was tracking in the ModuleLinker/IRMover. But can't retrieve it now, we want to import an alias to a global variable, should we import the global as well if it is link once?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:156
@@ +155,3 @@
+    // exported from outside as well.
+    for (auto &CalleeEdge : CalleeSummary->edges()) {
+      auto CalleeGUID = CalleeEdge.first;
----------------
This was intentional, comment added.


http://reviews.llvm.org/D18343