Skip to content

[SYCL] Always let the backend choose the binary #1587

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

Merged
bader merged 7 commits into from
May 12, 2020
Merged

[SYCL] Always let the backend choose the binary #1587

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bee1066
Always let the backend choose the binary
hiaselhans Apr 25, 2020
b7d6c64
set image type to spir64 for DynRTDeviceBinaryImage
hiaselhans Apr 28, 2020
86eb1e9
fix clang-format
hiaselhans Apr 28, 2020
a18ddcd
set BinaryImage's DeviceTargetSpec based on Format
hiaselhans Apr 28, 2020
4ef3f91
fix clang-format
hiaselhans Apr 28, 2020
3627035
another clang-format fix
hiaselhans Apr 28, 2020
b3ebb95
Merge branch 'sycl' into always_select_binary
hiaselhans May 10, 2020
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
30 changes: 19 additions & 11 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 @@ -665,9 +665,13 @@ void ProgramManager::populateSpecConstRegistry() {
RTDeviceBinaryImage &ProgramManager::getDeviceImage(OSModuleHandle M,
KernelSetId KSId,
const context &Context) {
if (DbgProgMgr > 0)
if (DbgProgMgr > 0) {
std::cerr << ">>> ProgramManager::getDeviceImage(" << M << ", \"" << KSId
<< "\", " << getRawSyclObjImpl(Context) << ")\n";

std::cerr << "available device images:\n";
Copy link

@Ruyk Ruyk Apr 29, 2020

Choose a reason for hiding this comment

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

Could help when filtering the output...

Suggested change
std::cerr << "available device images:\n";
std::cerr << "ProgramManager: Available device images:\n";

Copy link
Contributor Author

@hiaselhans hiaselhans Apr 29, 2020

Choose a reason for hiding this comment

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

actually i just moved this part up because it might be unreachable at it's old place.

If i were to improve those messages i would query for PI_TRACE at runtime instead? what do you think?

Copy link

@Ruyk Ruyk May 1, 2020

Choose a reason for hiding this comment

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

That would be even better

Copy link
Contributor Author

@hiaselhans hiaselhans May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@kbobrovs should i replace all DbgProgMgr checks with pi_trace in programmanager?

Copy link
Contributor

@kbobrovs kbobrovs May 12, 2020

Choose a reason for hiding this comment

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

@hiaselhans , sorry for delay. Yes, definitely makes sense. Can be done as a separate PR.

debugPrintBinaryImages();
}
std::lock_guard<std::mutex> Guard(Sync::getGlobalLock());
std::vector<RTDeviceBinaryImageUPtr> &Imgs = *m_DeviceImages[KSId];
const ContextImplPtr Ctx = getSyclObjImpl(Context);
Expand All @@ -680,19 +684,15 @@ RTDeviceBinaryImage &ProgramManager::getDeviceImage(OSModuleHandle M,

// Ask the native runtime under the given context to choose the device image
// it prefers.
if (Imgs.size() > 1) {
std::vector<pi_device_binary> RawImgs(Imgs.size());
for (unsigned I = 0; I < Imgs.size(); I++)
RawImgs[I] = const_cast<pi_device_binary>(&Imgs[I]->getRawData());
std::vector<pi_device_binary> RawImgs(Imgs.size());
for (unsigned I = 0; I < Imgs.size(); I++)
RawImgs[I] = const_cast<pi_device_binary>(&Imgs[I]->getRawData());

Ctx->getPlugin().call<PiApiKind::piextDeviceSelectBinary>(
getFirstDevice(Ctx), RawImgs.data(), (cl_uint)RawImgs.size(), &ImgInd);
}
Ctx->getPlugin().call<PiApiKind::piextDeviceSelectBinary>(
getFirstDevice(Ctx), RawImgs.data(), (cl_uint)RawImgs.size(), &ImgInd);
Img = Imgs[ImgInd].get();

if (DbgProgMgr > 0) {
std::cerr << "available device images:\n";
debugPrintBinaryImages();
std::cerr << "selected device image: " << &Img->getRawData() << "\n";
Img->print();
}
Expand Down Expand Up @@ -994,7 +994,6 @@ DynRTDeviceBinaryImage::DynRTDeviceBinaryImage(
Bin = new pi_device_binary_struct();
Bin->Version = PI_DEVICE_BINARY_VERSION;
Bin->Kind = PI_DEVICE_BINARY_OFFLOAD_KIND_SYCL;
Bin->DeviceTargetSpec = PI_DEVICE_BINARY_TARGET_UNKNOWN;
Bin->CompileOptions = "";
Bin->LinkOptions = "";
Bin->ManifestStart = nullptr;
Expand All @@ -1004,6 +1003,15 @@ DynRTDeviceBinaryImage::DynRTDeviceBinaryImage(
Bin->EntriesBegin = nullptr;
Bin->EntriesEnd = nullptr;
Bin->Format = pi::getBinaryImageFormat(Bin->BinaryStart, DataSize);

switch(Bin->Format) {
case PI_DEVICE_BINARY_TYPE_SPIRV:
Bin->DeviceTargetSpec = PI_DEVICE_BINARY_TARGET_SPIRV64;
break;
default:
Bin->DeviceTargetSpec = PI_DEVICE_BINARY_TARGET_UNKNOWN;
}
Copy link

@Ruyk Ruyk Apr 29, 2020

Choose a reason for hiding this comment

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

Does this work with the CUDA backend when there are multiple binaries?

Copy link
Contributor Author

@hiaselhans hiaselhans Apr 29, 2020

Choose a reason for hiding this comment

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

i think you can only load one DynRTDeviceBinaryImage at a time using the SYCL_USE_KERNEL_SPV env-variable. this just sets the devicetarget to spir64 in case it is a spir-file so the opencl backend can recognize it.


init(Bin);
}

Expand Down