[PATCH] D123225: [ThreadPool] add ability to group tasks into separate groups

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

I like the overall direction of this patch. I do see value in aganea's comments about possibly adding more methods to TaskGroup



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:68
+  template <typename Function, typename... Args>
+  inline auto async(TaskGroup &Group, Function &&F, Args &&...ArgList) {
+    auto Task =
----------------
aganea wrote:
> Do you think we could have all these `TaskGroup`-specific functions inside the `TaskGroup` class instead? As an alternative, given your current usage, the tasks could be even queued in a `std::vector` prior, and passed to the `TaskGroup` constructor.
I do like that idea, but if we do that it might be best to have TaskGroup take a "ThreadPool &" as a constructor argument so that it can ensure we always use the right ThreadPool if we ask the TaskGroup to wait. 

Do we need to lock down the task group once "wait()" is called with a TaskGroup or can users keep adding things to the TaskGroup even if work is currently going on? Should we freeze the contents of a TaskGroup once we start waiting on this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123225/new/

https://reviews.llvm.org/D123225