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

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 17 00:24:25 PDT 2016


chandlerc marked an inline comment as done.

================
Comment at: lib/Analysis/CodeMetrics.cpp:32-33
@@ +31,4 @@
+appendSpeculatableOperands(const Value *V,
+                           SmallPtrSetImpl<const Value *> &Visited,
+                           SmallVectorImpl<const Value *> &Worklist) {
+  const User *U = dyn_cast<User>(V);
----------------
majnemer wrote:
> I think you could use a `SetVector` here because you never pop from `Worklist`, right?
I started with that, but we add things to the visited set that we don't put on the worklist which allows us to avoid repeatedly querying isSafeToSpeculativelyExecute. While micro-optimizing the data structure seems unlikely to be worthwhile (see my comment below), avoiding repeated calls to this fairly heavy weight routine do seem worthwhile when the cost is just keeping the set separate.

================
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];
----------------
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. ;]


https://reviews.llvm.org/D23396





More information about the llvm-commits mailing list