Add an assertion that parallelForEach doesn't nest.

[ruiu]

Summary

Add an assertion that parallelForEach doesn't nest.

[andrewrk]

This will be nice to prevent bugs in the future. Should it be behind #ifndef NDEBUG though? Presumably if assertions are disabled then this bug would be expected to be undefined behavior and avoid the overhead of the thread local variable, is that right?

[ruiu]

If you worry about the cost of assigning a boolean value to the thread-local variable, I'd think that's really cheap and negligible. This function is not called too casually, because distributing jobs to threads waiting in the thread pool is not cheap. Compared to that, these assignments are virtually nothing.

[andrewrk]

That makes sense 👍

[MaskRay]

lld/Common/Threads.cpp

13

This doesn't have to be thread_local. This is a fundamental problem of TaskGroup.

template <class IterTy, class FuncTy>
void parallel_for_each(IterTy Begin, IterTy End, FuncTy Fn) {
  TaskGroup TG;
.... // In ~Latch called by ~TaskGroup, all threads of the default executor can block on different `std::mutex`es if there are multiple concurrent `parallel_for_each` invocations.
}

It doesn't need nested parallel_for_each to get stuck. It just needs more concurrent parallel_for_each invocations than the number of executors.

[MaskRay]

lld/Common/Threads.cpp

13

The code (with thread_local bool -> std::atomic<bool>) can probably be moved to include/llvm/Support/Parallel.h before it gets improved to allow concurrent invocations.

[ruiu]

lld/Common/Threads.cpp

13

Do you think you can fix the function so that parallel_for_each is reentrant? If so, do you mind if I ask you do that?

lld/Common/Threads.cpp

 //===- Threads.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Common/Threads.h"
+#include <thread>
 
 bool lld::ThreadsEnabled = true;
+thread_local bool lld::ParallelForEachRunning;

lld/include/lld/Common/Threads.h

 #define LLD_COMMON_THREADS_H
 
 #include "llvm/Support/Parallel.h"
 #include <functional>
 
 namespace lld {
 
 extern bool ThreadsEnabled;
+extern thread_local bool ParallelForEachRunning;
 
 template <typename R, class FuncTy> void parallelForEach(R &&Range, FuncTy Fn) {
+  // for_each is not reentrant, so guard against it.
+  assert(!ParallelForEachRunning);
+  ParallelForEachRunning = true;
+
   if (ThreadsEnabled)
     for_each(llvm::parallel::par, std::begin(Range), std::end(Range), Fn);
   else
     for_each(llvm::parallel::seq, std::begin(Range), std::end(Range), Fn);
+
+  ParallelForEachRunning = false;
 }
 
 inline void parallelForEachN(size_t Begin, size_t End,
                              llvm::function_ref<void(size_t)> Fn) {
+  assert(!ParallelForEachRunning);
+  ParallelForEachRunning = true;
+
   if (ThreadsEnabled)
     for_each_n(llvm::parallel::par, Begin, End, Fn);
   else
     for_each_n(llvm::parallel::seq, Begin, End, Fn);
+
+  ParallelForEachRunning = false;
 }
 
 template <typename R, class FuncTy> void parallelSort(R &&Range, FuncTy Fn) {
   if (ThreadsEnabled)
     sort(llvm::parallel::par, std::begin(Range), std::end(Range), Fn);
   else
     sort(llvm::parallel::seq, std::begin(Range), std::end(Range), Fn);
 }
 
 } // namespace lld
 
 #endif