Skip to content

Commit

Permalink
[SYCL] Ensure that RTDeviceBinaryImage instances have a unique image …
Browse files Browse the repository at this point in the history
…ID (#12526)

**Problem:**

Currently, the image id of an RTDeviceBinaryImage instance is simply the
pointer value of the underlying pi_device_binary (in
[getImageID(](https://github.com/intel/llvm/blob/sycl/sycl/source/detail/device_binary_image.hpp#L221))).
However, consider the following scenario:
1) We create a device image
2) Put into cache
3) Destroy the image (when it goes out of scope)
4) Create another image that _happens to be created at the same memory
address_ (thus having same image ID)

This causes two instances of RTDeviceBinaryImage to share the same image
id, which ends up causing a collision in the KernelProgramCache.

**Solution (Proposed in this PR)**

Have a counter in RTDeviceBinaryImage that increments upon instantiation
of this class. The counter value is added to the image id to ensure that
no two instances have the same ID.

**Alternative Solutions**

1. Remove the entry from the KernelProgramCache when the image is
destroyed. This solution would require more work as the
KernelProgramCache, currently, [does not support arbitrary element-wise
eviction](https://github.com/intel/llvm/blob/sycl/sycl/doc/design/KernelProgramCache.md#in-memory-cache-eviction)
(eviction follows a LRU strategy when cache size exceeds the threshold).
Moreover, I expect this to have additional performance overhead of
having to lock the cache and evicting. The proposed solution is much
more simpler.
  • Loading branch information
uditagarwal97 authored Jan 31, 2024
1 parent 2f20e37 commit 04ff5b8
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 1 deletion.
5 changes: 5 additions & 0 deletions sycl/source/detail/device_binary_image.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,13 @@ void RTDeviceBinaryImage::init(pi_device_binary Bin) {
DeviceGlobals.init(Bin, __SYCL_PI_PROPERTY_SET_SYCL_DEVICE_GLOBALS);
DeviceRequirements.init(Bin, __SYCL_PI_PROPERTY_SET_SYCL_DEVICE_REQUIREMENTS);
HostPipes.init(Bin, __SYCL_PI_PROPERTY_SET_SYCL_HOST_PIPES);

if (Bin)
ImageId = ImageCounter++;
}

std::atomic<uintptr_t> RTDeviceBinaryImage::ImageCounter = 1;

DynRTDeviceBinaryImage::DynRTDeviceBinaryImage(
std::unique_ptr<char[]> &&DataPtr, size_t DataSize)
: RTDeviceBinaryImage() {
Expand Down
6 changes: 5 additions & 1 deletion sycl/source/detail/device_binary_image.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ class RTDeviceBinaryImage {

std::uintptr_t getImageID() const {
assert(Bin && "Image ID is not available without a binary image.");
return reinterpret_cast<std::uintptr_t>(Bin);
return ImageId;
}

protected:
Expand All @@ -240,6 +240,10 @@ class RTDeviceBinaryImage {
RTDeviceBinaryImage::PropertyRange DeviceGlobals;
RTDeviceBinaryImage::PropertyRange DeviceRequirements;
RTDeviceBinaryImage::PropertyRange HostPipes;

private:
static std::atomic<uintptr_t> ImageCounter;
uintptr_t ImageId;
};

// Dynamically allocated device binary image, which de-allocates its binary
Expand Down

0 comments on commit 04ff5b8

Please sign in to comment.