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

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 23 19:32:37 PST 2015


tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:51
@@ +50,3 @@
+  auto &Module = ModuleMap[FileName];
+  if (!Module)
+    Module = loadFile(FileName, Context);
----------------
Sorry, is Y the module we are importing from or to in your example? If we are importing into it, we only load it once. If we are importing foo() and bar() from it, then yes, without caching the modules in between imports Y needs to be loaded twice, but only one function is parsed/materialized on each load. It is a time vs memory tradeoff that we'll have to evaluate.

Let me know if I didn't understand your scenario though.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:91
@@ +90,3 @@
+  /// transively called functions when importing.
+  SmallVector<Function *, 64> Worklist(CalledFunctions.begin(),
+                                       CalledFunctions.end());
----------------
joker.eph wrote:
> 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?
Yes. For example, in ModuleLinker::shouldLinkFromSource we will link from source (creating a new GV in the process with potentially new linkage type) when the dest is a declaration. We will even do this in certain cases when the source is only a declaration (weak linkage). So for example we create a new GV and delete the old dest GV for weakalias in your test case, which is an alias but is imported as a function decl since we aren't importing its aliasee. With the test case as-is I think we get lucky, but when I modified it I hit a seg fault somewhere because the Function in the work list is used after freed (apparently just luck that the memory was not yet overwritten).


http://reviews.llvm.org/D14914





More information about the llvm-commits mailing list