-
Notifications
You must be signed in to change notification settings - Fork 73
[KHR] Provide an alternative to <sycl/sycl.hpp> #814
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
base: main
Are you sure you want to change the base?
[KHR] Provide an alternative to <sycl/sycl.hpp> #814
Conversation
This PR aims to address KhronosGroup#780 by introducing a new KHR extension that document more headers that SYCL applications can include for better control of what they pay for in terms of compile time.
AlexeySachkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some comments for reviewers in advance.
See my talk at IWOCL 2025 for more background/context/motivation for this extension.
As of right now, we haven't prototyped the extension ourselves in Intel's SYCL implementation, but that will be done soon after first feedback on this proposal.
CTS haven't been updated yet to have a mode which uses this extension. What I discovered is that not that many CTS tests (in fact, just one (!)) test directly includes <sycl/sycl.hpp> header and the rest get it through things like common.h. So, before CTS can use this approach and benefit from it, some refactoring has to be done (which is somewhat already in motion, see my PRs like KhronosGroup/SYCL-CTS#1083 or KhronosGroup/SYCL-CTS#1077
Proper testing for this extension could actually be tricky. For example, Intel's SYCL headers are tightly interconnected and even if you include just one of them there is a high chance that you are getting a lot more. Therefore, its hard to use them to make sure that this specification did not forget about some class or function. I had an idea about taking synopsis headers from the spec and compiling CTS with them, but even without practically doing that I see that many synopsis headers are missing and it won't be the simplest thing to do (although it is probably a good exercise to make sure that CTS does not use any undocumented APIs).
Initial version of the proposal presented here roughly provides a separate header for almost every feature, bundling some of them together (or providing a higher-level aggregation header in some cases). From compile-time savings point of view it is not necessary to make some of those headers separately (like context, device, platform, etc. are very cheap), but it is not immediately clear how to bundle them together for simplicity of use (or whether it is needed at all).
Note that there are already some open issues recorded at the end of the proposed document.
| |This header provides access to functionality related to reductions (such as | ||
| [code]#reduction# interface and [code]#reducer class) | ||
|
|
||
| |[code]#<sycl/khr/includes/stream># | ||
| |Contains definition of [code]#stream# class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording that describes content of the headers changes all over the place. It took some time to prepare the PR and I was experimenting with different approaches as well.
I think that the first item here is to agree on the header contents and naming and then we can debate the wording describing them. Even though any immediate comments/suggestions about it are also welcome
| - [code]#get_native# | ||
| - [code]#make_*# | ||
|
|
||
| Note that even though functions defined in the header operate with various |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some kind of leftover from a previous version. The intent for this note is to apply to all headers, i.e. the extension does not mandate a strict split. It is a valid (even though useless) implementation option to just include existing <sycl/sycl.hpp> from every new header.
Therefore, the note should probably be moved into some common section and generalized.
Also highlighting this to double-check that everyone is on board with the suggested non-strict approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should go with the non-strict approach. I don't think there is precedent anywhere for "if you include X, I promise you won't also get Y", it would limit what implementations can do, and it would be really hard to test. I think phrasing that says "You must include <sycl/sycl.hpp> or <sycl/header> to guarantee features X, Y and Z are available" makes the most sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalized this note in ba0812d, also feedback on its wording is still welcome
| then any macro defined by other extensions myst be made available through | ||
| [code]#<sycl/khr/includes/version># header. | ||
|
|
||
| == Open issues/questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Immediately recording a point from @illuhad that we may want to have a header that provides forward-declarations only to gather feedback about this.
| Note that the simple swizzle functions ([code]#XYZW_SWIZZLE# and | ||
| [code]#RGBA_SWIZZLE# defined by the table 123) are only available when the macro | ||
| [code]#SYCL_SIMPLE_SWIZZLES# is defined before including | ||
| [code]#<sycl/khr/includes/vec>#. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could add a non-normative note here saying that it is recommended for developers to define SYCL_SIMPLE_SWIZZLES via the command-line and not in code, since this is the only way to guarantee that the macro is defined before the first include of vec (which may not be in user code)?
|
The topic was discussed at June 5th, 2025 SYCL WG call. My recollection of the discussion: Headers should have
Wording. Forward declarations. Let me know if anything is wrong or missing. Otherwise, I'm going to apply comments above to the PR |
|
I have prototyped this extension in SimSYCL. Only the following points that I think are worth discussing came up for me:
A nice side-effect of having individual headers is that it allows a better user experience in a few select scenarios. For example, if the user explicitly includes My detailed notes on each header
|
|
Thanks for the feedback, @PeterTh!
Yes, I think that it is already covered by the proposal:
I think that we need feedback from @Pennycook here. I'm not opposed to dropping it, neither I am opposed to having it. Both
My background with the split of those headers is to improve compile-times. From what I see in Intel's implementation is that those index space classes are quite small and cheap in terms of compile-time. And those classes oftentimes go together in user's applications. However, I'm looking once again at the data I gathered from This chart shows how many benchmarks that use feature Though, my data gathering approach wasn't perfect, i.e. it was a plain I'm not opposed to splitting this header, but I do not see much value in such split.
I can imagine that group algorithms are way heavier than any group class itself. However, what would be a good split here?
The intent here was that
Noted, I will take a deeper look into this header, I'm not very familiar with the reductions functionality.
I wonder if we want to have dedicated headers for different properties, because I have many questions when it comes to extensions. There could be ones which add new properties (and in fact, we have many of those at intel/llvm)
Similar questions are applicable to properties documented by the core spec, so it warrants a thorough look at them. |
I think we should split it up. I can't find where I posted it, but I said something to the effect of "We should provide the most fine-grained split we're comfortable with, and let users group things if they want to." If we don't have
What if we put the groups in I think it would make sense to put |
I agree. I think it makes more sense to include the properties along with the interface(s) that use them. For example, What would be the advantage of combining all of the properties into a single header? |
Added
Recorded suggested resolution in 7e6881d
Switched most of headers to use "provides" wording in 6aaa8df @gmlueck, I haven't resolved all your comments yet, but most of them are addressed. Items which are still on my TODO list: |
|
@PeterTh, I made an attempt at Properties were outlined into distinct headers in 59b5e14, see also #892 With those recent commits I believe that I've addressed all feedback. Whilst I wait for other comments, I will be working on revamping the prototype I had for intel/llvm and preparing a dedicated test sub-suite in the CTS for the extension |
|
Looks good to me! |
This reverts commit 59b5e14.
Would you be able to formally review it and give approval if you're happy, @PeterTh ? Thanks! |
| |Provides definition of the [code]#context# class. | ||
|
|
||
| |[code]#<sycl/khr/includes/index_space.hpp># | ||
| |Provides definition of the most index space identifiers from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| |Provides definition of the most index space identifiers from | |
| |Provides definitions of most of the index space identifiers from |
| class aliases. | ||
|
|
||
| |[code]#<sycl/khr/includes/functional.hpp># | ||
| |Provides definitions of function objects like [code]#plus# or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| |Provides definitions of function objects like [code]#plus# or | |
| |Provides definitions of function objects like [code]#plus# and |
| * [code]#usm::alloc# enumeration | ||
| * [code]#usm_allocator# class | ||
| * Free functions like [code]#malloc_device#, [code]#aligned_alloc_host#, | ||
| [code]#malloc# and [code]#get_pointer_type# as from sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [code]#malloc# and [code]#get_pointer_type# as from sections | |
| [code]#malloc# and [code]#get_pointer_type# from sections |
|
|
||
| Any extension which does not explicitly document how it can be accessed through | ||
| header files should be assumed to be available only through | ||
| [code]#<sycl/sycl.hpp>#. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to sketch out how our other KHRs will interact with this extension.
These KHRs only add new members to existing classes, so I presume they would not introduce any new headers:
- sycl_khr_default_context
- sycl_khr_queue_empty_query
- sycl_khr_queue_flush
The sycl_khr_group_interface KHR adds a bunch of new classes. I presume that KHR would provide a new header named <sycl/khr/group_interface.hpp>?
What about sycl_khr_max_work_group_queries? This KHR just adds two new device descriptors sycl::khr::info::device::max_work_group_range and khr::info::device::max_work_group_range_size. Would we add a new header file with just those two descriptor named <sycl/khr/max_work_group_queries.hpp>? Or, would we document that <sycl/khr/includes/device.hpp> also includes these descriptors?
| {endnote} | ||
|
|
||
| [[sec:khr-includes-version]] | ||
| === [code]#<sycl/khr/includes/version.hpp># header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TOC "level" is not right here. Currently, this and all the other sections that describe each header are under "Extension overview", which isn't right. I think there are two possibilities:
- Create a new section that is a peer of "Extension overview" named something like "New headers". Then, each section that describes a header will be under "New headers". Or,
- Raise the level of each of these sections, so they are all peers of "Extension overview".
I have a preference for the first option.
| This header contains definition of [code]#byte# type alias | ||
|
|
||
| [[sec:khr-includes-other-extensions]] | ||
| === Co-existence with other extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| === Co-existence with other extensions | |
| == Co-existence with other extensions |
This should not be under "Extension overview". If you decide to add a new section named "New headers", then it should not be under that either.
| If an implementation supports this extension, then any macro defined by other | ||
| supported extensions must be defined in [code]#<sycl/khr/includes/version.hpp>#. | ||
|
|
||
| == Open issues/questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not have any open issues, let's remove this section.
The specification can be found in KhronosGroup/SYCL-Docs#814 The implementation is a non-functional change for the existing codebase, but definitions of macro documented by the SYCL 2020 specifications were moved around a little bit to provide them all through the new `sycl/khr/version.hpp`. New headers are always present and their content is always available. However, we do not define `SYCL_KHR_INCLUDES` macro yet because the extension is still on review. That indicates that it is not formally supported by the implementation.
| * Backend macros in the form of [code]#SYCL_BACKEND_<backend_name># defined by | ||
| <<sec:backend-macros>>. | ||
|
|
||
| [code]#<sycl/khr/includes/version.hpp># header is included by every other header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor bikeshedding point: Is version.hpp the right name for this header? As I understand, it would provide all macro definitions of SYCL. It seems a bit confusing to me to have to include version.hpp if I want to get e.g. SYCL_EXTERNAL. Perhaps, macros.hpp might be more appropriate? Or since, it's included by all other headers, something more non-descriptive like core_defs.hpp?

This PR aims to address #780 by introducing a new KHR extension that document more headers that SYCL applications can include for better control of what they pay for in terms of compile time.