[PATCH] [Core] Add parallel infrastructure to lld.

Shankar Kalpathi Easwaran shankarke at gmail.com
Tue Apr 9 16:06:14 PDT 2013



================
Comment at: include/lld/Core/Parallel.h:103-104
@@ +102,4 @@
+      });
+      if (_stop)
+        break;
+      auto task = _workQueue.front();
----------------
You would need to unlock the mutex here.

================
Comment at: include/lld/Core/Parallel.h:73-81
@@ +72,11 @@
+    // take a while.
+    std::thread([&, threadCount] {
+      for (std::size_t i = 1; i < threadCount; ++i) {
+        std::thread([=] {
+          work();
+        }).detach();
+      }
+      work();
+    }).detach();
+  }
+
----------------
Nice!

================
Comment at: include/lld/Core/Parallel.h:68-69
@@ +67,4 @@
+public:
+  ThreadPoolExecutor(std::size_t threadCount =
+                         std::thread::hardware_concurrency())
+      : _stop(false), _done(threadCount) {
----------------
I think size_t is pretty big for a threadcount.

================
Comment at: include/lld/Core/Parallel.h:108
@@ +107,3 @@
+      lock.unlock();
+      task();
+    }
----------------
it would be nice to get the task return status and check for success/failure and set the stop field.

================
Comment at: include/lld/Core/Parallel.h:97
@@ +96,3 @@
+private:
+  void work() {
+    while (true) {
----------------
This could return an ErrorOr to indicate success/failure of this thread.

================
Comment at: include/lld/Core/Parallel.h:73-80
@@ +72,10 @@
+    // take a while.
+    std::thread([&, threadCount] {
+      for (std::size_t i = 1; i < threadCount; ++i) {
+        std::thread([=] {
+          work();
+        }).detach();
+      }
+      work();
+    }).detach();
+  }
----------------
Shankar Kalpathi Easwaran wrote:
> Nice!
Doesnot handle the case when some of the threads cant be created.

================
Comment at: include/lld/Core/Parallel.h:47-50
@@ +46,6 @@
+
+  void dec() {
+    if (--_count == 0)
+      _cond.notify_all();
+  }
+
----------------
you might want to add a else case to assert probably.

================
Comment at: include/lld/Core/Parallel.h:79
@@ +78,3 @@
+      }
+      work();
+    }).detach();
----------------
It might be good to add a name to the task, for tracing purposes.


http://llvm-reviews.chandlerc.com/D649



More information about the llvm-commits mailing list