Skip to content

[SYCL] Fix linking of kernel-bundles. #4398

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
merged 7 commits into from
Sep 5, 2021
Merged

[SYCL] Fix linking of kernel-bundles. #4398

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3f0fd8
seperate linking of kernel-bundles.
cperkinsintel Aug 24, 2021
dca69d6
added test to cover linking multiple kernels. This depends upon PR 4…
cperkinsintel Aug 30, 2021
49e0577
Merge branch 'intel-sycl' into cperkins-seperate-link-kernel-bundles
cperkinsintel Sep 1, 2021
df475e5
Merge branch 'intel-sycl' into cperkins-seperate-link-kernel-bundles
cperkinsintel Sep 2, 2021
ce7435b
moving test to llvm-test-suite
cperkinsintel Sep 2, 2021
9cf1a1b
inserted comment
cperkinsintel Sep 3, 2021
f31d62f
overlooked clang-format
cperkinsintel Sep 3, 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
18 changes: 13 additions & 5 deletions sycl/source/detail/kernel_bundle_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,13 @@ class kernel_bundle_impl {
// TODO: Unify with c'tor for sycl::comile and sycl::build by calling
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test to sycl/unittests/SYCL2020 ?

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 have a lit test for the multiple kernel stuff here: intel/llvm-test-suite#440
Is that what you are asking?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I was asking about adding a unit/component test in sycl/unittests/SYCL2020 directory of this repo. This test should be able to validate this behavior without any backend available.
Let's do it in a separate PR?

// sycl::join on vector of kernel_bundles

std::vector<device_image_plain> DeviceImages;
// The loop below just links each device image separately, not linking any
// two device images together. This is correct so long as each device image
// has no unresolved symbols. That's the case when device images are created
// from generic SYCL APIs. There's no way in generic SYCL to create a kernel
// which references an undefined symbol. If we decide in the future to allow
// a backend interop API to create a "sycl::kernel_bundle" that references
// undefined symbols, then the logic in this loop will need to be changed.
for (const kernel_bundle<bundle_state::object> &ObjectBundle :
ObjectBundles) {
for (const device_image_plain &DeviceImage : ObjectBundle) {
Expand All @@ -205,13 +211,15 @@ class kernel_bundle_impl {
}))
continue;

DeviceImages.insert(DeviceImages.end(), DeviceImage);
const std::vector<device_image_plain> VectorOfOneImage{DeviceImage};
std::vector<device_image_plain> LinkedResults =
detail::ProgramManager::getInstance().link(VectorOfOneImage,
MDevices, PropList);
MDeviceImages.insert(MDeviceImages.end(), LinkedResults.begin(),
LinkedResults.end());
}
}

MDeviceImages = detail::ProgramManager::getInstance().link(
std::move(DeviceImages), MDevices, PropList);

for (const kernel_bundle<bundle_state::object> &Bundle : ObjectBundles) {
const KernelBundleImplPtr BundlePtr = getSyclObjImpl(Bundle);
for (const std::pair<const std::string, std::vector<unsigned char>>
Expand Down