[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:180
+  GlobalValueSummary(SummaryKind K, GVFlags Flags, ArrayRef<ValueInfo> Refs)
+      : Kind(K), Flags(Flags), RefEdgeList(Refs) {}
 
----------------
mehdi_amini wrote:
> 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)
> 
> 
> 
> It is not clear to me why you think that "not simply using std::vector everywhere" is simpler though?
To avoid the copy we'd need to add a way to move the std::vector out of MapVector/SetVector. Not a big deal but it would make the interface for those classes a little more complicated.

> 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.
I'd say that copying the vector is the "default position" as it wouldn't require more API surface in MapVector/SetVector. It's also the status quo as we were effectively copying the data structure in using add*Edges before.

And while we're copying the vector we might as well use a slightly better data structure here.

But I can see how using OwningArrayRef here could be seen as taking a position on whether the memory savings are worth it, which I don't (see also other thread), so I guess I don't have a problem with moving the vector.


https://reviews.llvm.org/D27875