[PATCH] D14914: Add a FunctionImporter helper to perform summary-based cross-module function importing

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D14914#295184, @joker.eph wrote:

> Hi Teresa,
>
> Thanks for the review, I should have added in the description that I was mostly interested to share some high level design and APIs as I am working on ld64 integration. I know you're also working on the same functionality, I just wrote this one to be able to test cross-module importing without heuristic (i.e. import one level of the call graph unconditionally).
>
> So I don't mind at all dropping this patch if you already have a replacement (I'll continue to use this code for my prototype till you're ready), or if this patch is close enough to what you're working on we can integrate this and work iteratively to improve it. As you prefer :)


No problem, this is only a piece of the stuff I was working on (have the clang part as well), and is a bit simpler since I was trying to incorporate some initial profitability checking and iteratively handling the imported calls. I am thinking it is good to get this one in with the current functionality to serve as the general framework and I can add that part shortly. That way we can start iterating and refining this more quickly.

> Good point that the renaming is still missing, this is not gonna be trivial in the plugin I think as we will need to communicate the renaming and the new edges between file to the linker. Doing it in the FE would be nice, but I guess it won't be possible as it requires to have the index.


I am passing in the index to clang but this is to support distributed builds where we are invoking the backend piece as a separate job, so that won't help your use case. I will need to do something similar to what you are dealing with in the gold-plugin for supporting single machine ThinLTO builds too. 

> This is why I'm glad I posted this as it already exposes some interface with the linker I'm missing at that time.

> 

> Thanks,

> 

> Mehdi



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:105
@@ +104,3 @@
+    }
+    // This should not happen, could we assert here?
+    if (InfoList->second.empty()) {
----------------
joker.eph wrote:
> tejohnson wrote:
> > We should always have the function info populated, even if we are doing lazy parsing of summaries. The case where we decided not to include a function in the index (because we decided earlier that it was not eligible for or profitable to import) should be handled by the above check on the returned iterator.
> IIUC the answer is "yes we could assert here", right?
Yep, sorry, I meant you can assert here.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:159
@@ +158,3 @@
+    // Link in the specified function.
+    if (L.linkInModule(&Module, Linker::Flags::None, &Index, F))
+      report_fatal_error("Function Import: link error");
----------------
joker.eph wrote:
> tejohnson wrote:
> > After this, we need to walk the imported function copy in the destination and find new callsites that need to be imported. For now if you want to add a FIXME comment to that effect it is fine and I can add that along with profitability heuristics. Otherwise you will end up importing all functions in the call tree below functions in this module...
> Yes I didn't want to import the full call-graph, so I limited arbitrarily to one level.
> I guess we'll need to move to a `worklist` pattern for this loop and enqueue imported function?
Yep, I think that is what is going to be needed (e.g. some kind of priority queue), but go ahead and leave as-is for now, that can be added when the functionality for adding imported callsites is added.



http://reviews.llvm.org/D14914