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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Hi Mehdi,

Didn't realize you were working on this, I was readying a slightly different version. We can work from your version though. Some initial comments below.

There are a couple of pieces missing. One thing that is missing here is to import for calls from imported functions, noted below. Another thing is that there potentially needs to be static promotion/renaming in the current module being compiled. I have a clang FE patch to handle that part, since it should be done early.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:50
@@ +49,3 @@
+Module &FunctionImporter::getOrLoadModule(StringRef FileName) {
+  auto &Module = ModuleMap[FileName];
+  if (!Module)
----------------
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.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:81
@@ +80,3 @@
+            CalledFunctions.insert(
+                cast<CallInst>(I).getCalledFunction()->getName());
+        }
----------------
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.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:95
@@ +94,3 @@
+    // Do not try to import locally defined function.
+    if (DefinedFunctions.count(CalledFunctionName))
+      continue;
----------------
Not needed as noted above.

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

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:111
@@ +110,3 @@
+    }
+    // Comdat can have multiple entries, FIXME: what do we do with them?
+    auto &Info = InfoList->second[0];
----------------
With my comdat importing patch we probably shouldn't even hit this situation as I made a change to import referenced comdat groups. Fine to pick the first entry here for now though, we can reevaluate later.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:119
@@ +118,3 @@
+
+    auto *Summary = Info->functionSummary();
+    if (!Summary) {
----------------
Add a note that we should eventually check if we are doing lazy parsing of summaries, in which case we import lazily at this point.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:144
@@ +143,3 @@
+
+      //FIXME this can happens with alias for instance, to be implemented...
+      continue;
----------------
Need to get the function for the alias base object. Here's the code from my version of the importer, you can probably just drop this in and modify the variable names:

    GlobalValue *SGV = SrcM->getNamedValue(FunctionName);
    Function *SF = dyn_cast<Function>(SGV);
    if (!SF && isa<GlobalAlias>(SGV)) {
      auto *SGA = dyn_cast<GlobalAlias>(SGV);
      SF = dyn_cast<Function>(SGA->getBaseObject());
    }


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


http://reviews.llvm.org/D14914