LLVMReactor: Drop codegenMutex, now thread safe.
LLVMReactor used to have a Big Fat Global Mutex over the entire
lifetime of the Nucleus object. This was required as LLVMReactor
used global variables for storing builder state.
Over the past year, there has been significant code cleanup and
global state has been reduced to a couple of globals that can now
be marked thread_local. With all state now being immutable global
or thread local, we are now able to remove the mutex.
ASAN and TSAN checks for our unittests are clean.
Bug: b/153803432
Change-Id: Ibe4019fb783f86e734387db431539e915369b488
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/33484
Tested-by: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
diff --git a/src/Reactor/Coroutine.hpp b/src/Reactor/Coroutine.hpp
index d70ecf0..cd8763c 100644
--- a/src/Reactor/Coroutine.hpp
+++ b/src/Reactor/Coroutine.hpp
@@ -135,12 +135,10 @@
// executable code. After calling, no more reactor functions may be
// called without building a new rr::Function or rr::Coroutine.
// While automatically called by operator(), finalize() should be called
- // as early as possible to release the global Reactor mutex lock.
- // It must also be called explicitly on the same thread that instantiates
- // the Coroutine instance if operator() is invoked on separate threads.
- // This is because presently, Reactor backends use a global mutex scoped
- // to the generation of routines, and these must be locked/unlocked on the
- // same thread.
+ // as soon as possible once the coroutine has been fully built.
+ // finalize() *must* be called explicitly on the same thread that
+ // instantiates the Coroutine instance if operator() is to be invoked on
+ // different threads.
inline void finalize(const Config::Edit &cfg = Config::Edit::None);
// Starts execution of the coroutine and returns a unique_ptr to a
diff --git a/src/Reactor/LLVMJIT.cpp b/src/Reactor/LLVMJIT.cpp
index bbd91e3..68153a7 100644
--- a/src/Reactor/LLVMJIT.cpp
+++ b/src/Reactor/LLVMJIT.cpp
@@ -57,6 +57,7 @@
__pragma(warning(pop))
#endif
+#include <atomic>
#include <unordered_map>
#if defined(_WIN64)
@@ -610,7 +611,7 @@
for(size_t i = 0; i < count; i++)
{
auto func = funcs[i];
- static size_t numEmittedFunctions = 0;
+ static std::atomic<size_t> numEmittedFunctions = { 0 };
std::string name = "f" + llvm::Twine(numEmittedFunctions++).str();
func->setName(name);
func->setLinkage(llvm::GlobalValue::ExternalLinkage);
diff --git a/src/Reactor/LLVMReactor.cpp b/src/Reactor/LLVMReactor.cpp
index 6cce3f2..48861e9 100644
--- a/src/Reactor/LLVMReactor.cpp
+++ b/src/Reactor/LLVMReactor.cpp
@@ -59,10 +59,13 @@
}
#endif
+#if !LLVM_ENABLE_THREADS
+# error "LLVM_ENABLE_THREADS needs to be enabled"
+#endif
+
namespace {
-std::unique_ptr<rr::JITBuilder> jit;
-std::mutex codegenMutex;
+thread_local std::unique_ptr<rr::JITBuilder> jit;
// Default configuration settings. Must be accessed under mutex lock.
std::mutex defaultConfigLock;
@@ -599,8 +602,6 @@
Nucleus::Nucleus()
{
- ::codegenMutex.lock(); // Reactor and LLVM are currently not thread safe
-
ASSERT(jit == nullptr);
jit.reset(new JITBuilder(Nucleus::getDefaultConfig()));
}
@@ -608,7 +609,6 @@
Nucleus::~Nucleus()
{
jit.reset();
- ::codegenMutex.unlock();
}
void Nucleus::setDefaultConfig(const Config &cfg)
diff --git a/src/Reactor/Reactor.cpp b/src/Reactor/Reactor.cpp
index dff1a29..2bf5324 100644
--- a/src/Reactor/Reactor.cpp
+++ b/src/Reactor/Reactor.cpp
@@ -63,7 +63,7 @@
}
// Set of variables that do not have a stack location yet.
-std::unordered_set<Variable *> Variable::unmaterializedVariables;
+thread_local std::unordered_set<Variable *> Variable::unmaterializedVariables;
Variable::Variable(Type *type, int arraySize)
: arraySize(arraySize)
diff --git a/src/Reactor/Reactor.hpp b/src/Reactor/Reactor.hpp
index 488c0be..33e8b44 100644
--- a/src/Reactor/Reactor.hpp
+++ b/src/Reactor/Reactor.hpp
@@ -133,7 +133,7 @@
static void materializeAll();
static void killUnmaterialized();
- static std::unordered_set<Variable *> unmaterializedVariables;
+ static thread_local std::unordered_set<Variable *> unmaterializedVariables;
Type *const type;
mutable Value *rvalue = nullptr;
diff --git a/src/Reactor/ReactorUnitTests.cpp b/src/Reactor/ReactorUnitTests.cpp
index d5aafa9..adb0cb4 100644
--- a/src/Reactor/ReactorUnitTests.cpp
+++ b/src/Reactor/ReactorUnitTests.cpp
@@ -2949,6 +2949,114 @@
EXPECT_EQ(result, value);
}
+TEST(ReactorUnitTests, Multithreaded_Function)
+{
+ constexpr int numThreads = 32;
+ constexpr int numLoops = 64;
+
+ auto threads = std::unique_ptr<std::thread[]>(new std::thread[numThreads]);
+ auto results = std::unique_ptr<int[]>(new int[numThreads * numLoops]);
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ auto threadFunc = [&](int t) {
+ for(int l = 0; l < numLoops; l++)
+ {
+ FunctionT<int(int, int)> function;
+ {
+ Int a = function.Arg<0>();
+ Int b = function.Arg<1>();
+ Return((a << 16) | b);
+ }
+
+ auto f = function("thread%d_loop%d", t, l);
+ results[t * numLoops + l] = f(t, l);
+ }
+ };
+ threads[t] = std::thread(threadFunc, t);
+ }
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ threads[t].join();
+ }
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ for(int l = 0; l < numLoops; l++)
+ {
+ auto expect = (t << 16) | l;
+ auto result = results[t * numLoops + l];
+ EXPECT_EQ(result, expect);
+ }
+ }
+}
+
+TEST(ReactorUnitTests, Multithreaded_Coroutine)
+{
+ if(!rr::Caps.CoroutinesSupported)
+ {
+ SUCCEED() << "Coroutines not supported";
+ return;
+ }
+
+ constexpr int numThreads = 32;
+ constexpr int numLoops = 64;
+
+ struct Result
+ {
+ bool yieldReturns[3];
+ int yieldValues[3];
+ };
+
+ auto threads = std::unique_ptr<std::thread[]>(new std::thread[numThreads]);
+ auto results = std::unique_ptr<Result[]>(new Result[numThreads * numLoops]);
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ auto threadFunc = [&](int t) {
+ for(int l = 0; l < numLoops; l++)
+ {
+ Coroutine<int(int, int)> function;
+ {
+ Int a = function.Arg<0>();
+ Int b = function.Arg<1>();
+ Yield(a);
+ Yield(b);
+ }
+
+ auto coroutine = function(t, l);
+
+ auto &result = results[t * numLoops + l];
+ result = {};
+ result.yieldReturns[0] = coroutine->await(result.yieldValues[0]);
+ result.yieldReturns[1] = coroutine->await(result.yieldValues[1]);
+ result.yieldReturns[2] = coroutine->await(result.yieldValues[2]);
+ }
+ };
+ threads[t] = std::thread(threadFunc, t);
+ }
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ threads[t].join();
+ }
+
+ for(int t = 0; t < numThreads; t++)
+ {
+ for(int l = 0; l < numLoops; l++)
+ {
+ auto const &result = results[t * numLoops + l];
+ EXPECT_EQ(result.yieldReturns[0], true);
+ EXPECT_EQ(result.yieldValues[0], t);
+ EXPECT_EQ(result.yieldReturns[1], true);
+ EXPECT_EQ(result.yieldValues[1], l);
+ EXPECT_EQ(result.yieldReturns[2], false);
+ EXPECT_EQ(result.yieldValues[2], 0);
+ }
+ }
+}
+
int main(int argc, char **argv)
{
::testing::InitGoogleTest(&argc, argv);
