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] Implement get_backend_info() #12906

Merged
merged 18 commits into from
Mar 29, 2024
Merged

Conversation

HPS-1
Copy link
Contributor

@HPS-1 HPS-1 commented Mar 5, 2024

Implementing the get_backend_info() functions for our SYCL implementation based on SYCL 2020 spec. (Link here: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html you may search for "get_backend_info()" there for the spec for these functions)
There're six groups of variations for this function, namely sycl::platform::get_backend_info(), sycl::context::get_backend_info(), sycl::device::get_backend_info(), sycl::queue::get_backend_info(), sycl::eventv::get_backend_info(), and sycl::kernel::get_backend_info()

One known concern: it seems that sycl::platform, sycl::context and sycl::kernel may have multiple associated device, but according to the spec the return type for sycl::xxx::get_backend_info<info::device::version>() should be std::string (i.e. a single device version) so I'm just returning the version of the first associated device in the list. Is this OK?

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1 HPS-1 changed the title Implement get_backend_info() [SYCL] Implement get_backend_info() Mar 7, 2024
@HPS-1 HPS-1 marked this pull request as ready for review March 7, 2024 15:44
@HPS-1 HPS-1 requested a review from a team as a code owner March 7, 2024 15:44
@HPS-1 HPS-1 requested a review from againull March 7, 2024 15:44
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Tests are missing. We should at least add some to sycl/test which would check that we can compile code using new API and/or there could be tests in sycl/test-e2e which would check that the new API behaves as expected

sycl/source/detail/context_impl.cpp Outdated Show resolved Hide resolved
…-coded return types with ones associated with traits

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1
Copy link
Contributor Author

HPS-1 commented Mar 18, 2024

Tests are missing. We should at least add some to sycl/test which would check that we can compile code using new API and/or there could be tests in sycl/test-e2e which would check that the new API behaves as expected

Fixed! Added the new e2e test case sycl/test-e2e/backend_info.cpp for this purpose.

…nst non-OpenCL backends

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

@HPS-1

One known concern: it seems that sycl::platform, sycl::context and sycl::kernel may have multiple associated device, but according to the spec the return type for sycl::xxx::get_backend_infoinfo::device::version() should be std::string (i.e. a single device version) so I'm just returning the version of the first associated device in the list. Is this OK?

info::device::version is the information descriptor of the sycl::device. Do we have to actually support it for all other classes sycl::platform, sycl::context and sycl::kernel? Maybe it's enough to support it only for sycl::device.

sycl/source/detail/event_impl.cpp Show resolved Hide resolved
sycl/include/sycl/info/sycl_backend_traits.def Outdated Show resolved Hide resolved
sycl/source/detail/event_impl.cpp Outdated Show resolved Hide resolved
@HPS-1
Copy link
Contributor Author

HPS-1 commented Mar 20, 2024

@HPS-1

One known concern: it seems that sycl::platform, sycl::context and sycl::kernel may have multiple associated device, but according to the spec the return type for sycl::xxx::get_backend_infoinfo::device::version() should be std::string (i.e. a single device version) so I'm just returning the version of the first associated device in the list. Is this OK?

info::device::version is the information descriptor of the sycl::device. Do we have to actually support it for all other classes sycl::platform, sycl::context and sycl::kernel? Maybe it's enough to support it only for sycl::device.

@againull Hi! Thanks for reviewing the code. And about the question you have, the thing is that the SYCL specification for (all variations of) get_backend_info() says "The type alias Param::return_type must be defined in accordance with the SYCL backend specification.", while the SYCL backend spec (for OpenCL, link here https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_backend_specific_information_descriptors) suggest that both info::platform::version and info::device::version are backend-specific info descriptors. I think this means that we should allow the user to query them using all variations of get_backend_info(), including the ones you mentioned such as sycl::context::get_backend_info().

@againull
Copy link
Contributor

againull commented Mar 20, 2024

@HPS-1

One known concern: it seems that sycl::platform, sycl::context and sycl::kernel may have multiple associated device, but according to the spec the return type for sycl::xxx::get_backend_infoinfo::device::version() should be std::string (i.e. a single device version) so I'm just returning the version of the first associated device in the list. Is this OK?

info::device::version is the information descriptor of the sycl::device. Do we have to actually support it for all other classes sycl::platform, sycl::context and sycl::kernel? Maybe it's enough to support it only for sycl::device.

@againull Hi! Thanks for reviewing the code. And about the question you have, the thing is that the SYCL specification for (all variations of) get_backend_info() says "The type alias Param::return_type must be defined in accordance with the SYCL backend specification.", while the SYCL backend spec (for OpenCL, link here https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_backend_specific_information_descriptors) suggest that both info::platform::version and info::device::version are backend-specific info descriptors. I think this means that we should allow the user to query them using all variations of get_backend_info(), including the ones you mentioned such as sycl::context::get_backend_info().

In such case probably we can return the version of the default/selected device.
For example, let's say if we have 3 devices (opencl gpu, l0 gpu, opencl cpu) in the context.
If user didn't set ONEAPI_DEVICE_SELECTOR, then we can return version of the device chosen by default selector - it will be l0 gpu.
If user set ONEAPI_DEVICE_SELECTOR=opencl:cpu, then we can return the version of the opencl cpu device.

But maybe it worth discussing with language group.

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@againull againull self-requested a review March 28, 2024 23:07
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@againull againull merged commit b9aa33e into intel:sycl Mar 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants