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

Michael Spencer bigcheesegs at gmail.com
Tue Apr 9 16:35:47 PDT 2013



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

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

================
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) {
----------------
Shankar Kalpathi Easwaran wrote:
> I think size_t is pretty big for a threadcount.
I'll change it to the return type of hardware_concurrency (unsigned).

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

================
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:
> Shankar Kalpathi Easwaran wrote:
> > Nice!
> Doesnot handle the case when some of the threads cant be created.
We don't use exception handling. There's no way to detect this case.

================
Comment at: include/lld/Core/Parallel.h:97
@@ +96,3 @@
+private:
+  void work() {
+    while (true) {
----------------
Shankar Kalpathi Easwaran wrote:
> This could return an ErrorOr to indicate success/failure of this thread.
It can't fail. And what would look at the return value?

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


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



More information about the llvm-commits mailing list