Skip to content

[SYCL] Remove unnecessary kernel ID protector mutex #4537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[SYCL] Remove unnecessary kernel ID protector mutex #4537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af831bd
[SYCL] Remove unnecessary kernel ID protector mutex
steffenlarsen Sep 10, 2021
2f5beb5
Added note about write-access limitation
steffenlarsen Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions sycl/source/detail/program_manager/program_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,6 @@ void ProgramManager::addImages(pi_device_binaries DeviceBinary) {
// ... or create the set first if it hasn't been
KernelSetId KSId = getNextKernelSetId();
{
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);
for (_pi_offload_entry EntriesIt = EntriesB; EntriesIt != EntriesE;
++EntriesIt) {
auto Result = KSIdMap.insert(std::make_pair(EntriesIt->name, KSId));
Expand Down Expand Up @@ -1259,16 +1258,12 @@ static bool compatibleWithDevice(RTDeviceBinaryImage *BinImage,
}

kernel_id ProgramManager::getSYCLKernelID(const std::string &KernelName) {
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);

auto KernelID = m_KernelIDs.find(KernelName);
assert(KernelID != m_KernelIDs.end() && "Kernel ID missing");
return KernelID->second;
}

std::vector<kernel_id> ProgramManager::getAllSYCLKernelIDs() {
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);

std::vector<sycl::kernel_id> AllKernelIDs;
AllKernelIDs.reserve(m_KernelIDs.size());
for (std::pair<std::string, kernel_id> KernelID : m_KernelIDs) {
Expand Down Expand Up @@ -1329,7 +1324,6 @@ ProgramManager::getSYCLDeviceImagesWithCompatibleState(
pi_device_binary DevBin =
const_cast<pi_device_binary>(&BinImage->getRawData());
{
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);
for (_pi_offload_entry EntriesIt = DevBin->EntriesBegin;
EntriesIt != DevBin->EntriesEnd; ++EntriesIt) {
auto KernelID = m_KernelIDs.find(EntriesIt->name);
Expand Down
8 changes: 1 addition & 7 deletions sycl/source/detail/program_manager/program_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,15 +275,9 @@ class ProgramManager {
/// Maps names of kernels to their unique kernel IDs.
/// TODO: Use std::unordered_set with transparent hash and equality functions
/// when C++20 is enabled for the runtime library.
/// Access must be guarded by the m_KernelIDsMutex mutex
/// Write access is only allowed during start-up (addImages).
std::unordered_map<std::string, kernel_id> m_KernelIDs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note should be placed here saying that write access is only allowed during start-up process in addImages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I've added a note.


/// Protects kernel ID cache.
/// NOTE: This may be acquired while \ref Sync::getGlobalLock() is held so to
/// avoid deadlocks care must be taken not to acquire
/// \ref Sync::getGlobalLock() while holding this mutex.
std::mutex m_KernelIDsMutex;

// Keeps track of pi_program to image correspondence. Needed for:
// - knowing which specialization constants are used in the program and
// injecting their current values before compiling the SPIR-V; the binary
Expand Down