[PATCH] D23396: [Assumptions] Make collecting ephemeral values not quadratic in the number of assume intrinsics.

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/CodeMetrics.cpp:54
@@ +53,3 @@
+  // worklist forever.
+  for (int i = 0; i < (int)Worklist.size(); ++i) {
+    const Value *V = Worklist[i];
----------------
chandlerc wrote:
> hfinkel wrote:
> > chandlerc wrote:
> > > hfinkel wrote:
> > > > Is there a reason to use the index here? Why not write this as:
> > > > 
> > > >   for (auto I = Worklist.begin(); I != Worklist.end(); ++I) {
> > > >     const Value *V = *I;
> > > > 
> > > Because that isn't valid.
> > > 
> > > When you grow the worklist you invalidate all iterators (because the iterators are just pointers and its a vector that may have to re-allocate).
> > Ah, right ;) -- Why don't we use a std::deque for these things?
> Because std::deque is *amazingly* slower, especially for the common case of a small number of values. And any deque implementation will have this property really. Too much book-keeping for too little value.
> 
> As Danny pointed out, far and away the best data structure patterns we will come up with will look roughly like a double stack or a vector that auto-shrinks the front when forced to re-allocate due to growth at the back.
> 
> But I strongly, strongly suspect that *this* is not the code path so hot to motivate adding a highly tuned data structure. So I don't think we should wait for that here, I think we should go with the normal, boring solution that is used pretty much everywhere else already. And if/when we find something that is hot enough to *really* care, let's build a nicely tuned thing.
> 
> All I really want is to make this not quadratic. ;]
> Because std::deque is *amazingly* slower, especially for the common case of a small number of values. 
> ...
> But I strongly, strongly suspect that *this* is not the code path so hot to motivate adding a highly tuned data structure.

So this is kind of my point ;) -- but fair enough, if this is the useful general pattern, this LGTM.



https://reviews.llvm.org/D23396