[PATCH] D13847: llvm-lto support for generating combined function indexes

[Mehdi AMINI via llvm-commits]

joker.eph accepted this revision.
joker.eph added a comment.
This revision is now accepted and ready to land.

LGTM (modulo a few minor comments).


================
Comment at: test/tools/llvm-lto/thinlto.ll:1
@@ +1,2 @@
+; RUN: llvm-as -function-summary %s -o %t.o
+; RUN: llvm-as -function-summary %p/Inputs/thinlto.ll -o %t2.o
----------------
Can you put a comment a the top of the file saying what's the point of this test?

================
Comment at: tools/llvm-lto/llvm-lto.cpp:183
@@ +182,3 @@
+
+/// \brief Create a combined index file from the input IR files and write it.
+///
----------------
I believe we have autobrief and you can leave the `\brief` tag out.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:189
@@ +188,3 @@
+  LLVMContext Context;
+  // Context.setDiagnosticHandler(handleDiagnostics, nullptr, true);
+  std::unique_ptr<FunctionInfoIndex> CombinedIndex(new FunctionInfoIndex());
----------------
?

================
Comment at: tools/llvm-lto/llvm-lto.cpp:190
@@ +189,3 @@
+  // Context.setDiagnosticHandler(handleDiagnostics, nullptr, true);
+  std::unique_ptr<FunctionInfoIndex> CombinedIndex(new FunctionInfoIndex());
+  uint64_t NextModuleId = 0;
----------------
Could a local variable be enough?


http://reviews.llvm.org/D13847