From 98ad98cbc01f54309330d4695afe4ca4ecdd60b9 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Wed, 26 Feb 2020 14:48:34 +0300 Subject: [PATCH 1/3] [SYCL] Add a mutex to state-dependent program functions This is done in order to consistently report errors if the user tries to compile/link/build the same program from multiple threads. Signed-off-by: Sergey Semenov --- sycl/source/detail/program_impl.cpp | 8 ++++++++ sycl/source/detail/program_impl.hpp | 2 ++ 2 files changed, 10 insertions(+) diff --git a/sycl/source/detail/program_impl.cpp b/sycl/source/detail/program_impl.cpp index 3125008515487..bc417b400967a 100644 --- a/sycl/source/detail/program_impl.cpp +++ b/sycl/source/detail/program_impl.cpp @@ -14,6 +14,7 @@ #include #include #include +#include __SYCL_INLINE_NAMESPACE(cl) { namespace sycl { @@ -42,7 +43,9 @@ program_impl::program_impl( DevicesSorted = sort_devices_by_cl_device_id(MDevices); } check_device_feature_support(MDevices); + vector_class> Locks; for (const auto &Prg : ProgramList) { + Locks.emplace_back(Prg->MMutex); Prg->throw_if_state_is_not(program_state::compiled); if (Prg->MContext != MContext) { throw invalid_object_error( @@ -162,6 +165,7 @@ cl_program program_impl::get() const { void program_impl::compile_with_kernel_name(string_class KernelName, string_class CompileOptions, OSModuleHandle M) { + std::unique_lock Lock(MMutex); throw_if_state_is_not(program_state::none); MProgramModuleHandle = M; if (!is_host()) { @@ -173,6 +177,7 @@ void program_impl::compile_with_kernel_name(string_class KernelName, void program_impl::compile_with_source(string_class KernelSource, string_class CompileOptions) { + std::unique_lock Lock(MMutex); throw_if_state_is_not(program_state::none); // TODO should it throw if it's host? if (!is_host()) { @@ -185,6 +190,7 @@ void program_impl::compile_with_source(string_class KernelSource, void program_impl::build_with_kernel_name(string_class KernelName, string_class BuildOptions, OSModuleHandle Module) { + std::unique_lock Lock(MMutex); throw_if_state_is_not(program_state::none); MProgramModuleHandle = Module; if (!is_host()) { @@ -205,6 +211,7 @@ void program_impl::build_with_kernel_name(string_class KernelName, void program_impl::build_with_source(string_class KernelSource, string_class BuildOptions) { + std::unique_lock Lock(MMutex); throw_if_state_is_not(program_state::none); // TODO should it throw if it's host? if (!is_host()) { @@ -215,6 +222,7 @@ void program_impl::build_with_source(string_class KernelSource, } void program_impl::link(string_class LinkOptions) { + std::unique_lock Lock(MMutex); throw_if_state_is_not(program_state::compiled); if (!is_host()) { check_device_feature_support(MDevices); diff --git a/sycl/source/detail/program_impl.hpp b/sycl/source/detail/program_impl.hpp index 9580d2fb20994..7f56287bcb21f 100644 --- a/sycl/source/detail/program_impl.hpp +++ b/sycl/source/detail/program_impl.hpp @@ -20,6 +20,7 @@ #include #include #include +#include __SYCL_INLINE_NAMESPACE(cl) { namespace sycl { @@ -364,6 +365,7 @@ class program_impl { RT::PiProgram MProgram = nullptr; program_state MState = program_state::none; + std::mutex MMutex; ContextImplPtr MContext; bool MLinkable = false; vector_class MDevices; From f0794e937314264e84435915e941b70d75996e94 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Tue, 14 Apr 2020 11:59:33 +0300 Subject: [PATCH 2/3] Address comments Signed-off-by: Sergey Semenov --- sycl/source/detail/program_impl.cpp | 32 ++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/sycl/source/detail/program_impl.cpp b/sycl/source/detail/program_impl.cpp index 2c12f7bf4c3bc..f979ab62dfdfa 100644 --- a/sycl/source/detail/program_impl.cpp +++ b/sycl/source/detail/program_impl.cpp @@ -39,6 +39,24 @@ program_impl::program_impl( throw runtime_error("Non-empty vector of programs expected", PI_INVALID_VALUE); } + + // Sort the programs to avoid deadlocks due to locking multiple mutexes & + // verify that all programs are unique. + std::sort(ProgramList.begin(), ProgramList.end(), + [](const shared_ptr_class &A, + const shared_ptr_class &B) { + return A.get() < B.get(); + }); + auto It = std::unique(ProgramList.begin(), ProgramList.end(), + [](const shared_ptr_class &A, + const shared_ptr_class &B) { + return A.get() == B.get(); + }); + if (It != ProgramList.end()) { + throw runtime_error("Attempting to link a program with itself", + PI_INVALID_PROGRAM); + } + MContext = ProgramList[0]->MContext; MDevices = ProgramList[0]->MDevices; vector_class DevicesSorted; @@ -46,9 +64,9 @@ program_impl::program_impl( DevicesSorted = sort_devices_by_cl_device_id(MDevices); } check_device_feature_support(MDevices); - vector_class> Locks; + vector_class>> Locks; for (const auto &Prg : ProgramList) { - Locks.emplace_back(Prg->MMutex); + Locks.emplace_back(new std::lock_guard(Prg->MMutex)); Prg->throw_if_state_is_not(program_state::compiled); if (Prg->MContext != MContext) { throw invalid_object_error( @@ -187,7 +205,7 @@ cl_program program_impl::get() const { void program_impl::compile_with_kernel_name(string_class KernelName, string_class CompileOptions, OSModuleHandle M) { - std::unique_lock Lock(MMutex); + std::lock_guard Lock(MMutex); throw_if_state_is_not(program_state::none); MProgramModuleHandle = M; if (!is_host()) { @@ -199,7 +217,7 @@ void program_impl::compile_with_kernel_name(string_class KernelName, void program_impl::compile_with_source(string_class KernelSource, string_class CompileOptions) { - std::unique_lock Lock(MMutex); + std::lock_guard Lock(MMutex); throw_if_state_is_not(program_state::none); // TODO should it throw if it's host? if (!is_host()) { @@ -212,7 +230,7 @@ void program_impl::compile_with_source(string_class KernelSource, void program_impl::build_with_kernel_name(string_class KernelName, string_class BuildOptions, OSModuleHandle Module) { - std::unique_lock Lock(MMutex); + std::lock_guard Lock(MMutex); throw_if_state_is_not(program_state::none); MProgramModuleHandle = Module; if (!is_host()) { @@ -233,7 +251,7 @@ void program_impl::build_with_kernel_name(string_class KernelName, void program_impl::build_with_source(string_class KernelSource, string_class BuildOptions) { - std::unique_lock Lock(MMutex); + std::lock_guard Lock(MMutex); throw_if_state_is_not(program_state::none); // TODO should it throw if it's host? if (!is_host()) { @@ -244,7 +262,7 @@ void program_impl::build_with_source(string_class KernelSource, } void program_impl::link(string_class LinkOptions) { - std::unique_lock Lock(MMutex); + std::lock_guard Lock(MMutex); throw_if_state_is_not(program_state::compiled); if (!is_host()) { check_device_feature_support(MDevices); From acada7e0555d142ce4e0636c50b74463273e6d58 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Tue, 14 Apr 2020 15:48:24 +0300 Subject: [PATCH 3/3] Apply more comments Signed-off-by: Sergey Semenov --- sycl/source/detail/program_impl.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/sycl/source/detail/program_impl.cpp b/sycl/source/detail/program_impl.cpp index f979ab62dfdfa..a6f65f51f3682 100644 --- a/sycl/source/detail/program_impl.cpp +++ b/sycl/source/detail/program_impl.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -42,16 +43,8 @@ program_impl::program_impl( // Sort the programs to avoid deadlocks due to locking multiple mutexes & // verify that all programs are unique. - std::sort(ProgramList.begin(), ProgramList.end(), - [](const shared_ptr_class &A, - const shared_ptr_class &B) { - return A.get() < B.get(); - }); - auto It = std::unique(ProgramList.begin(), ProgramList.end(), - [](const shared_ptr_class &A, - const shared_ptr_class &B) { - return A.get() == B.get(); - }); + std::sort(ProgramList.begin(), ProgramList.end()); + auto It = std::unique(ProgramList.begin(), ProgramList.end()); if (It != ProgramList.end()) { throw runtime_error("Attempting to link a program with itself", PI_INVALID_PROGRAM); @@ -64,9 +57,9 @@ program_impl::program_impl( DevicesSorted = sort_devices_by_cl_device_id(MDevices); } check_device_feature_support(MDevices); - vector_class>> Locks; + std::list> Locks; for (const auto &Prg : ProgramList) { - Locks.emplace_back(new std::lock_guard(Prg->MMutex)); + Locks.emplace_back(Prg->MMutex); Prg->throw_if_state_is_not(program_state::compiled); if (Prg->MContext != MContext) { throw invalid_object_error(