[PATCH] D18763: [ThinLTO] Move summary computation from BitcodeWriter to new pass

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/Analysis/ModuleSummaryAnalysis.h:40
@@ +39,3 @@
+  /// Constructor that builds an index for the given Module.
+  ModuleSummaryIndexBuilder(const Module *M, Pass *P = nullptr);
+
----------------
Pass argument requires some good documentation I think.

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:123
@@ -84,3 +122,3 @@
   /// are listed in the derived FunctionSummary object.
-  std::vector<GlobalValue::GUID> RefEdgeList;
+  std::vector<ValueInfo> RefEdgeList;
 
----------------
I'm not super enthusiastic about this: we're leaking the bitcode implementation details here. I don't have a great suggestion at that time unfortunately :(

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:124
@@ +123,3 @@
+                                llvm::make_unique<GlobalValueInfo>(GVInfo));
+}
+
----------------
I think my representation of aliases will make this obsolete, just as all the "clone()" method on the `GlobalValueInfo` right?

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:126
@@ +125,3 @@
+
+ModuleSummaryIndexBuilder::ModuleSummaryIndexBuilder(const Module *M, Pass *P)
+    : Index(llvm::make_unique<ModuleSummaryIndex>()), M(M) {
----------------
Using a pass as an argument is really not nice here, and also it is never passed at call site.
Can you pas instead a `std::function<BlockFrequencyInfo *(Function &F)>` that returns either a pointer to the BFI for the function F, or `nullptr` if not available?

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:144
@@ +143,3 @@
+    else if (F.getEntryCount().hasValue()) {
+      Function &Func = const_cast<Function &>(F);
+      LoopInfo LI{DominatorTree(Func)};
----------------
Why is this `const_cast` needed?

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:198
@@ +197,3 @@
+bool ModuleSummaryIndexWrapperPass::runOnModule(Module &M) {
+  IndexBuilder = llvm::make_unique<ModuleSummaryIndexBuilder>(&M);
+  return false;
----------------
I think you intended to pass an extra argument here.

================
Comment at: lib/Bitcode/Writer/BitcodeWriterPass.cpp:25
@@ +24,3 @@
+  if (EmitSummaryIndex)
+    Index = ModuleSummaryIndexBuilder(&M).takeIndex();
+  WriteBitcodeToFile(&M, OS, ShouldPreserveUseListOrder, Index.get());
----------------
Here as well, extra argument.


http://reviews.llvm.org/D18763