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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:175
   /// are listed in the derived FunctionSummary object.
-  std::vector<ValueInfo> RefEdgeList;
+  OwningArrayRef<ValueInfo> RefEdgeList;
 
----------------
mehdi_amini wrote:
> I didn't know about `OwningArrayRef`, interesting.
> What is the advantage over a `const std::vector` ?
I guess it saves us a pointer as well as the additional space reserved by the `std::vector`, since the size is fixed. I haven't measured how important this is in practice, though.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:180
+  GlobalValueSummary(SummaryKind K, GVFlags Flags, ArrayRef<ValueInfo> Refs)
+      : Kind(K), Flags(Flags), RefEdgeList(Refs) {}
 
----------------
mehdi_amini wrote:
> 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.
I guess there's one way that saves copies and another that saves memory. In the absence of evidence pointing one way or the other, I'd be inclined to go with the slightly simpler approach, i.e. the one I took.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:138
         // aliasee.
-        auto *CalleeId =
-            M.getValueSymbolTable().lookup(CalledValue->getName());
+        auto *CalleeId = cast<GlobalValue>(CalledValue);
 
----------------
tejohnson wrote:
> CalleeId doesn't seem like the best name anymore. Just "Callee"?
I folded it into the only use.


https://reviews.llvm.org/D27875