[Support] Attempt to fix deadlock in ThreadGroup

[aganea]

Summary

This is an attempt to fix the situation described by https://reviews.llvm.org/D104207#2826290 and PR41508.

See sequence of operations leading to the bug in https://reviews.llvm.org/D104207#3004689

We ensure that the Latch is completely "free" before decrementing the number of TaskGroupInstances.

[MaskRay]

llvm/lib/Support/Parallel.cpp

155

L is a member of TaskGroup. Hasn't its destructor already run sync?

[mehdi_amini]

llvm/lib/Support/Parallel.cpp

155

~TaskGroup() is invoked before destructing the individual members of the TaskGroup class.

[MaskRay]

Hmm. ~Latch should be deleted, then.

[aganea]

Removed the sync from ~Latch().
Removed the std::condition_variable::notify_all() improvement since it was occasionally breaking on Windows (and Linux too it seems).

[aganea]

Could you please take a second look?

I've removed the change to std::condition_variable::notify_all() because it was introducing a new issue, where the Latch object would be destroyed in-between the release of the mutex in Latch::dec() and the call to notify_all() in the same function. This is probably because the sleeping thread was awaken spuriously before notify_all() was even called, like mentioned here:

https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-sleepconditionvariablecs

Condition variables are subject to spurious wakeups (those not associated with an explicit wake) and stolen wakeups (another thread manages to run before the woken thread). Therefore, you should recheck a predicate (typically in a while loop) after a sleep operation returns.

[mehdi_amini]

LGTM

[MaskRay]

LG

[andrewng]

Managed to end up here after looking at the query on D70447. Should this patch be cherry picked to the llvm-13 release branch?

[mehdi_amini]

In D109914#3121623, @andrewng wrote:

Managed to end up here after looking at the query on D70447. Should this patch be cherry picked to the llvm-13 release branch?

Fine with me, I think you need to file a bug for @tstellar

llvm/include/llvm/Support/Parallel.h

 
 class Latch {
   uint32_t Count;
   mutable std::mutex Mutex;
   mutable std::condition_variable Cond;
 
 public:
   explicit Latch(uint32_t Count = 0) : Count(Count) {}
-  ~Latch() { sync(); }
+  ~Latch() {
+    // Ensure at least that sync() was called.
+    assert(Count == 0);
+  }
 
   void inc() {
     std::lock_guard<std::mutex> lock(Mutex);
     ++Count;
   }
 
   void dec() {
     std::lock_guard<std::mutex> lock(Mutex);

llvm/lib/Support/Parallel.cpp

 
 static std::atomic<int> TaskGroupInstances;
 
 // Latch::sync() called by the dtor may cause one thread to block. If is a dead
 // lock if all threads in the default executor are blocked. To prevent the dead
 // lock, only allow the first TaskGroup to run tasks parallelly. In the scenario
 // of nested parallel_for_each(), only the outermost one runs parallelly.
 TaskGroup::TaskGroup() : Parallel(TaskGroupInstances++ == 0) {}
-TaskGroup::~TaskGroup() { --TaskGroupInstances; }
+TaskGroup::~TaskGroup() {
+  // We must ensure that all the workloads have finished before decrementing the
+  // instances count.
+  L.sync();
+  --TaskGroupInstances;
+}
 
 void TaskGroup::spawn(std::function<void()> F) {
   if (Parallel) {
     L.inc();
     Executor::getDefaultExecutor()->add([&, F] {
       F();
       L.dec();
     });