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

[Teresa Johnson via llvm-commits]

tejohnson 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);
+
----------------
joker.eph wrote:
> Pass argument requires some good documentation I think.
Will change to call back and document.

================
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;
 
----------------
joker.eph wrote:
> 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 :(
Only in the sense that it motivates the need for a union. The constituent types (Value* and GUID) are not bitcode specific.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:124
@@ +123,3 @@
+                                llvm::make_unique<GlobalValueInfo>(GVInfo));
+}
+
----------------
joker.eph wrote:
> I think my representation of aliases will make this obsolete, just as all the "clone()" method on the `GlobalValueInfo` right?
Yes, this should go away along with the clone() support. I can remove all of this since your follow on will presumably go in shortly after this one.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:126
@@ +125,3 @@
+
+ModuleSummaryIndexBuilder::ModuleSummaryIndexBuilder(const Module *M, Pass *P)
+    : Index(llvm::make_unique<ModuleSummaryIndex>()), M(M) {
----------------
joker.eph wrote:
> 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?
The not passing was inadvertent as you point out below. But I like the callback approach better. 

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:144
@@ +143,3 @@
+    else if (F.getEntryCount().hasValue()) {
+      Function &Func = const_cast<Function &>(F);
+      LoopInfo LI{DominatorTree(Func)};
----------------
joker.eph wrote:
> Why is this `const_cast` needed?
Because the BlockFrequencyInfo (or BranchProbabilityInfo, can't remember which one) takes a non-const F. When called through the pass the Module is not const. But when this class is constructed from llvm-as the Module is constant so the Module pointer is constant in the builder. 

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

================
Comment at: lib/Bitcode/Writer/BitcodeWriterPass.cpp:25
@@ +24,3 @@
+  if (EmitSummaryIndex)
+    Index = ModuleSummaryIndexBuilder(&M).takeIndex();
+  WriteBitcodeToFile(&M, OS, ShouldPreserveUseListOrder, Index.get());
----------------
joker.eph wrote:
> Here as well, extra argument.
No, this one not derived from Pass. As an aside, I have no idea in the new pass manager, which I believe is what BitcodeWriterPass is, how to do the equivalent of getAnalysis<>. So right now it will construct the BFI as it does when called via llvm-as.


http://reviews.llvm.org/D18763