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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for the review, responses below.


================
Comment at: include/llvm/Transforms/IPO/PassManagerBuilder.h:120
@@ -117,1 +119,3 @@
+  std::string FunctionImportIndexFile;
+
   bool DisableTailCalls;
----------------
joker.eph wrote:
> 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.
Yes and this is related to a TODO I added to the companion clang patch D15025. Since we have to create a function index there (or from the gold-plugin or whatever linker is being used in the single machine version), I think we should stash the FunctionIndex on the dest Module. In that case we could use the existing index off the module if it is non-null, or the -summary-file if not. I will make that change.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:243
@@ -242,1 +242,3 @@
+  /// File holding the function summary index for import
+  StringRef IndexFile;
 
----------------
joker.eph wrote:
> 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)`)
Ok agreed. But will be moot with the above change to stash index on module.

================
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;
----------------
joker.eph wrote:
> This test+error is redundant with the one 3 lines below I think.
No, this is testing for when the user has supplied an index twice (via front-end and via -summary-file). The below error is for when the user has supplied neither. I.e. together they ensure the index must be specified exactly once.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:292
@@ -277,1 +291,3 @@
+  return new FunctionImportPass(IndexFile);
+}
 }
----------------
joker.eph wrote:
> It would be nice to keep the ability to create the pass without the IndexFile, so overload instead of replace here?
Or just make the parameter have a default of "" as in the FunctionImportPass constructor change I made. But this will be moot with my change to stash the index on the module.


http://reviews.llvm.org/D15024