[PATCH] D15024: [ThinLTO] Support for specifying function index from pass manager

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/Transforms/IPO.h:88
@@ -87,1 +87,3 @@
 
+ModulePass *createFunctionImportPass(StringRef IndexFile);
+
----------------
Document.

================
Comment at: include/llvm/Transforms/IPO/PassManagerBuilder.h:120
@@ -117,1 +119,3 @@
+  std::string FunctionImportIndexFile;
+
   bool DisableTailCalls;
----------------
Could we pass and store here directly the FunctionIndex in memory instead of a path to the file?
I can imagine some use cases where we have it in memory and want to construct the pass pipeline (ld64 plugin for instance), and the interface  shouldn't force to dump to disk.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:243
@@ -242,1 +242,3 @@
+  /// File holding the function summary index for import
+  StringRef IndexFile;
 
----------------
Storing a string ref is error prone: the client needs to keep it alive. I'd rather store a std::string (and have all the prototype `createFunctionImportPass(std::string)`)

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:258
@@ +257,3 @@
+      if (!IndexFile.empty())
+        report_fatal_error("error: -summary-file and file from frontend\n");
+      IndexFile = SummaryFile;
----------------
This test+error is redundant with the one 3 lines below I think.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:292
@@ -277,1 +291,3 @@
+  return new FunctionImportPass(IndexFile);
+}
 }
----------------
It would be nice to keep the ability to create the pass without the IndexFile, so overload instead of replace here?


http://reviews.llvm.org/D15024