Skip to content
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.

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

[SYCL] Select device image based on compile_target device image property #14909

Merged
merged 18 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 65 additions & 13 deletions sycl/source/detail/program_manager/program_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,26 @@ void CheckJITCompilationForImage(const RTDeviceBinaryImage *const &Image,
}
}

const char *getArchName(const device &Device) {
namespace syclex = sycl::ext::oneapi::experimental;
auto Arch = Device.get_info<syclex::info::device::architecture>();
switch (Arch) {
#define __SYCL_ARCHITECTURE(ARCH, VAL) \
case syclex::architecture::ARCH: \
return #ARCH;
#define __SYCL_ARCHITECTURE_ALIAS(ARCH, VAL)
#include <sycl/ext/oneapi/experimental/architectures.def>
#undef __SYCL_ARCHITECTURE
#undef __SYCL_ARCHITECTURE_ALIAS
}
return "unknown";
}

sycl_device_binary getRawImg(RTDeviceBinaryImage *Img) {
return reinterpret_cast<sycl_device_binary>(
const_cast<sycl_device_binary>(&Img->getRawData()));
}

template <typename StorageKey>
RTDeviceBinaryImage *getBinImageFromMultiMap(
const std::unordered_multimap<StorageKey, RTDeviceBinaryImage *> &ImagesSet,
Expand All @@ -1262,16 +1282,51 @@ RTDeviceBinaryImage *getBinImageFromMultiMap(
if (ItBegin == ItEnd)
return nullptr;

std::vector<sycl_device_binary> RawImgs(std::distance(ItBegin, ItEnd));
auto It = ItBegin;
for (unsigned I = 0; It != ItEnd; ++It, ++I)
RawImgs[I] = reinterpret_cast<sycl_device_binary>(
const_cast<sycl_device_binary>(&It->second->getRawData()));
// Here, we aim to select all the device images from the
// [ItBegin, ItEnd) range that are AOT compiled for Device
// (checked using info::device::architecture) or JIT compiled.
// This selection will then be passed to urDeviceSelectBinary
// for final selection.
std::string_view ArchName = getArchName(Device);
std::vector<RTDeviceBinaryImage *> DeviceFilteredImgs;
DeviceFilteredImgs.reserve(std::distance(ItBegin, ItEnd));
for (auto It = ItBegin; It != ItEnd; ++It) {
auto PropRange = It->second->getDeviceRequirements();
auto PropIt =
std::find_if(PropRange.begin(), PropRange.end(), [&](const auto &Prop) {
return Prop->Name == std::string_view("compile_target");
});
auto AddImg = [&]() { DeviceFilteredImgs.push_back(It->second); };

std::vector<ur_device_binary_t> UrBinaries(RawImgs.size());
for (uint32_t BinaryCount = 0; BinaryCount < RawImgs.size(); BinaryCount++) {
UrBinaries[BinaryCount].pDeviceTargetSpec =
getUrDeviceTarget(RawImgs[BinaryCount]->DeviceTargetSpec);
// Device image has no compile_target property, so it is JIT compiled.
if (PropIt == PropRange.end()) {
AddImg();
maarquitos14 marked this conversation as resolved.
Show resolved Hide resolved
continue;
}

// Device image has the compile_target property, so it is AOT compiled for
// some device, check if that architecture is Device's architecture.
auto CompileTargetByteArray = DeviceBinaryProperty(*PropIt).asByteArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is asCString helper:

const char *asCString() const;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the property data guaranteed to be null terminated though? From my understanding the memory layout of these property values come from the PropertyValue class, which I don't think explicitly null terminates the data.

PropertyValue(const llvm::StringRef &Str)
: PropertyValue(reinterpret_cast<const byte *>(Str.data()),
Str.size() * sizeof(char) * /* bits in one byte */ 8) {}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the property data guaranteed to be null terminated though?

Good point. I still think that using asCString would be a better fit, but I assume it requires an update to the helper itself to be made safe first. Therefore, that seems like an improvement that can/should be done separately

CompileTargetByteArray.dropBytes(8);
std::string_view CompileTarget(
reinterpret_cast<const char *>(&CompileTargetByteArray[0]),
CompileTargetByteArray.size());
// Note: there are no explicit targets for CPUs, so on x86_64,
// so we use a spir64_x86_64 compile target image.
if ((ArchName == CompileTarget) ||
(ArchName == "x86_64" && CompileTarget == "spir64_x86_64")) {
jzc marked this conversation as resolved.
Show resolved Hide resolved
AddImg();
}
}

if (DeviceFilteredImgs.empty())
return nullptr;

std::vector<ur_device_binary_t> UrBinaries(DeviceFilteredImgs.size());
for (uint32_t BinaryCount = 0; BinaryCount < DeviceFilteredImgs.size();
BinaryCount++) {
UrBinaries[BinaryCount].pDeviceTargetSpec = getUrDeviceTarget(
getRawImg(DeviceFilteredImgs[BinaryCount])->DeviceTargetSpec);
}

uint32_t ImgInd = 0;
Expand All @@ -1280,8 +1335,7 @@ RTDeviceBinaryImage *getBinImageFromMultiMap(
getSyclObjImpl(Context)->getPlugin()->call(
urDeviceSelectBinary, getSyclObjImpl(Device)->getHandleRef(),
UrBinaries.data(), UrBinaries.size(), &ImgInd);
std::advance(ItBegin, ImgInd);
return ItBegin->second;
return DeviceFilteredImgs[ImgInd];
}

RTDeviceBinaryImage &
Expand Down Expand Up @@ -1310,10 +1364,8 @@ ProgramManager::getDeviceImage(const std::string &KernelName,
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);
if (auto KernelId = m_KernelName2KernelIDs.find(KernelName);
KernelId != m_KernelName2KernelIDs.end()) {
// Kernel ID presence guarantees that we have bin image in the storage.
Img = getBinImageFromMultiMap(m_KernelIDs2BinImage, KernelId->second,
Context, Device);
assert(Img && "No binary image found for kernel id");
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that this assert is effectively replaced by an assert within vector::operator[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this assert because it is now not guaranteed the Img will be non null in this case. Specifically, there could be no image compatible with the current device, and getBinImageFromMultiMap will return nullptr:
https://github.com/intel/llvm/pull/14909/files#diff-78dd7f7ba0b6120dece1ae4ab5a09c9936ff654a1de2c31ff2dbb1fc58d90393R1308-R1309
In this case, the control flow then flows down to the exception. This flow is demonstrated by the NoDeviceKernel test.

} else {
Img = getBinImageFromMultiMap(m_ServiceKernels, KernelName, Context,
Device);
Expand Down
9 changes: 8 additions & 1 deletion sycl/unittests/helpers/UrImage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,14 @@ inline UrImage
generateDefaultImage(std::initializer_list<std::string> KernelNames) {
UrPropertySet PropSet;

std::vector<unsigned char> Bin{0, 1, 2, 3, 4, 5}; // Random data
std::string Combined;
for (auto it = KernelNames.begin(); it != KernelNames.end(); ++it) {
if (it != KernelNames.begin())
Combined += ", ";
Combined += *it;
}
std::vector<unsigned char> Bin(Combined.begin(), Combined.end());
Bin.push_back(0);

UrArray<UrOffloadEntry> Entries = makeEmptyKernels(KernelNames);

Expand Down
1 change: 1 addition & 0 deletions sycl/unittests/program_manager/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
set(CMAKE_CXX_EXTENSIONS OFF)
add_sycl_unittest(ProgramManagerTests OBJECT
CompileTarget.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be alphabetically sorted? Not sure really, some of the elements don't seem to be.

BuildLog.cpp
DynamicLinking.cpp
itt_annotations.cpp
Expand Down
Loading
Loading