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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

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 :)

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. 
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:50
@@ +49,3 @@
+Module &FunctionImporter::getOrLoadModule(StringRef FileName) {
+  auto &Module = ModuleMap[FileName];
+  if (!Module)
----------------
tejohnson wrote:
> I was not planning to keep all the modules being imported from in memory, to minimize the memory overhead. That does require reparsing the initial part of the bitcode, but that is quite fast in practice.
> 
> The metadata importing patch handles this by not parsing/materializing metadata until a separate metadata parse/link step. Note that even if we are to save the modules as in this approach we would need to delay the metadata parsing/linking until the end of all importing to reduce memory overhead, so much of that patch is still needed.
Note that these are only the "Lazy" modules used for importing. They should consume less memory since they don't initialize the bodies.
That said, I agree that some measurements need to be done to explore the cost tradeoff.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:81
@@ +80,3 @@
+            CalledFunctions.insert(
+                cast<CallInst>(I).getCalledFunction()->getName());
+        }
----------------
tejohnson wrote:
> I think this should save the called function pointers in a set, rather than the name. You also don't need the DefinedFunctions set. Just check here if the called function is a declaration, and don't add to the called function set if not.
Thanks good point.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:105
@@ +104,3 @@
+    }
+    // This should not happen, could we assert here?
+    if (InfoList->second.empty()) {
----------------
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?

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


http://reviews.llvm.org/D14914