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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:50
@@ +49,3 @@
+Module &FunctionImporter::getOrLoadModule(StringRef FileName) {
+  auto &Module = ModuleMap[FileName];
+  if (!Module)
----------------
joker.eph wrote:
> 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.
> 
About caching the Module, the question is also if I have a call to foo() and bar() in module Y, both function being defined in module Y, this allows to transparently load Y only once when importing into Y.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:91
@@ +90,3 @@
+  /// transively called functions when importing.
+  SmallVector<Function *, 64> Worklist(CalledFunctions.begin(),
+                                       CalledFunctions.end());
----------------
tejohnson wrote:
> I just hit an issue with the earlier version of this patch I had downloaded and modified to store a worklist of Function pointers instead, and a slightly modified test case. The module linker may replace the dest module Function (erasing it from the parent) if it creates a new GV while linking global value protos. It might do this for other declarations it finds in the import module besides the import function. I had forgotten about this, I was dealing with it in my version of the import pass by passing in the structure to update with the new Function. But that is probably overkill. So unfortunately I think we may need to go back to tracking the names of function decls instead of the Function pointers.
Do you mean that while importing function `foo()` into module A, the Linker can change a declaration for function `bar()`, invalidating the Function pointer to `bar()` that may be in the worklist?


http://reviews.llvm.org/D14914