[PATCH] D27875: IR: Eliminate non-determinism in the module summary analysis.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:45
-/// Struct to hold value either by GUID or Value*, depending on whether this
-/// is a combined or per-module index, respectively.
 struct ValueInfo {
----------------
The comment is outdated: IndirectCalls in per-module index are GUID-based.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:175
   /// are listed in the derived FunctionSummary object.
-  std::vector<ValueInfo> RefEdgeList;
+  OwningArrayRef<ValueInfo> RefEdgeList;
 
----------------
I didn't know about `OwningArrayRef`, interesting.
What is the advantage over a `const std::vector` ?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:180
+  GlobalValueSummary(SummaryKind K, GVFlags Flags, ArrayRef<ValueInfo> Refs)
+      : Kind(K), Flags(Flags), RefEdgeList(Refs) {}
 
----------------
It seems to me that this is causing memory allocation and copy that could be avoided: because Refs is an `ArrayRef` we're allocating and copying into `RefEdgeList`. However looking at call site, if `Refs` comes from `makeRefList` we just created a vector there. If it comes from `findRefEdges` we just created a `SetVector`.

In both cases it looks like we could take and store a `std::vector` that would be moved in.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:92
-  DenseMap<const Value *, CalleeInfo> CallGraphEdges;
-  DenseMap<GlobalValue::GUID, CalleeInfo> IndirectCallEdges;
-  DenseSet<const Value *> RefEdges;
----------------
Fusing these looks much more sane!


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4798
+ModuleSummaryIndexBitcodeReader::makeRefList(ArrayRef<uint64_t> Record) {
+  std::vector<ValueInfo> Ret;
+  for (uint64_t RefValueId : Record)
----------------
`Ret.reserve( Record.size());`


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4806
+    ArrayRef<uint64_t> Record, bool IsOldProfileFormat, bool HasProfile) {
+  std::vector<FunctionSummary::EdgeTy> Ret;
+  for (unsigned I = 0, E = Record.size(); I != E; ++I) {
----------------
`Ret.reserve( Record.size());`


https://reviews.llvm.org/D27875