[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:175
   /// are listed in the derived FunctionSummary object.
-  std::vector<ValueInfo> RefEdgeList;
+  OwningArrayRef<ValueInfo> RefEdgeList;
 
----------------
pcc wrote:
> 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.
Oh makes sense! There is no "capacity" so you save a pointer (2 instead of 3).



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:180
+  GlobalValueSummary(SummaryKind K, GVFlags Flags, ArrayRef<ValueInfo> Refs)
+      : Kind(K), Flags(Flags), RefEdgeList(Refs) {}
 
----------------
pcc wrote:
> 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.
It is not clear to me why you think that "not simply using std::vector everywhere" is simpler though? 

This means hitting the heap a lot without good reason (that I perceive right now), which we traditionally avoid as much as possible I believe (reusing SmallVector across records in the BitcodeReader for example). Because of that it does not seem "one way or another to me":  adding this extra malloc/free/copy sequence for saving a pointer should be motivated.

(And I just saved 8B for this structure in D27970, if memory was a concern we would have started with this)





https://reviews.llvm.org/D27875