[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/IR/ModuleSummaryIndex.h:123
@@ -84,3 +122,3 @@
   /// are listed in the derived FunctionSummary object.
-  std::vector<GlobalValue::GUID> RefEdgeList;
+  std::vector<ValueInfo> RefEdgeList;
 
----------------
tejohnson wrote:
> 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.
Oh for some reason I thought we will end up with the value ID, pointers are fine.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:144
@@ +143,3 @@
+    else if (F.getEntryCount().hasValue()) {
+      Function &Func = const_cast<Function &>(F);
+      LoopInfo LI{DominatorTree(Func)};
----------------
tejohnson wrote:
> 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. 
Ok I update BFI upstream, but DomTree seems a lot more involved...

You should be able to update to:

```
      LoopInfo LI{DominatorTree(const_cast<Function &>(F))};
      BranchProbabilityInfo BPI{F, LI};
      BFIPtr = llvm::make_unique<BlockFrequencyInfo>(F, BPI, LI);
```

================
Comment at: lib/Bitcode/Writer/BitcodeWriterPass.cpp:25
@@ +24,3 @@
+  if (EmitSummaryIndex)
+    Index = ModuleSummaryIndexBuilder(&M).takeIndex();
+  WriteBitcodeToFile(&M, OS, ShouldPreserveUseListOrder, Index.get());
----------------
tejohnson wrote:
> 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.
ok


http://reviews.llvm.org/D18763