[PATCH] D48240: Try again to implement a FIFO task queue

Pavel Labath via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 22 03:51:35 PDT 2018


labath added a comment.

I think I noticed one more bug in the !threads implementation. Other than that, this looks fine to me, but I'll leave the final ok to Chandler.



================
Comment at: llvm/include/llvm/Support/TaskQueue.h:144-150
+    auto WrappedTask = [this, Id] {
+      assert(!Tasks.empty());
+      do {
+        Tasks.front()();
+        Tasks.pop_front();
+      } while (Id != NextRunId++);
+    };
----------------
I just realized this could be wrong when the futures are waited on out of order. E.g. if you have F1 and F2, `F2.wait()` will increment `NextRunId` to 2. Then if you do `F1.wait()`, it will loop until NextRunId wraps back to 1 and crash in the process.

Fortunately, with RunIds it should be easy to fix this by changing `!=` to `<`. If we're worried about uint64_t wrapping we can do something like `Id-NextRunId < UINT64_MAX/2`

(Also, this code could be moved into a separate function to decrease the number of instantiations of the lambda.)


https://reviews.llvm.org/D48240





More information about the llvm-commits mailing list