Repository navigation
[SYCL] Add sycl_khr_properties container and classification traits - #23075
Conversation
|
@koparasy the khr extension you are referring to is in draft state. Do you expect it to be reviewed and merged in advance? @dm-vodopyanov, @gmlueck I am not aware about process for this case, could you please clarify? |
@KseniyaTikhomirova the KHR is expected to be updated and be undrafted (soonish). I don't know the process, but the code is guarded with the |
KseniyaTikhomirova
left a comment
KseniyaTikhomirova
left a comment
There was a problem hiding this comment.
I have found that we have a process for this https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/KHRExtensions.md
To me it looks like draft/unmerged PRs against main are not intended to be merged in advance. Although I don't work with extensions so may be wrong here. @dm-vodopyanov kindly ping for your opinion.
changes itself LGTM, just a few comments
|
|
||
| // is_property_key_for / is_property_for / is_property_list_for. | ||
| static_assert(is_property_key_for_v<enable_profiling_key, MyClass>); | ||
| static_assert(!is_property_key_for_v<enable_profiling_key, OtherClass>); |
There was a problem hiding this comment.
would be nice to have this check for all types of properties
Cover is_property_key_v for the compile-time-type key, check is_property_key_for/is_property_for for every property kind, and add a mixed-class property list to verify is_property_list_for is false when properties target different classes.
Requested changes are done in d53a64d @dm-vodopyanov any opinions about the "khr process"? |
dm-vodopyanov
left a comment
dm-vodopyanov
left a comment
There was a problem hiding this comment.
In accordance with https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/KHRExtensions.md, we can merge this patch right now with enabled __DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS macro.
A few notes from this doc why it's crucial to merge such PRs:
So, we can have a PR but can't merge it. Keeping PRs opened for a long time is a
bad practice, because they tend to get stale: there are merge conflicts,
potential functional issues due to the codebase changes, etc.
In order for us to avoid stale PRs, all functionality which is a public
interface of an "in-progress" KHR extension, must be hidden under
`__DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS` macro. That way we can merge a PR to
avoid constantly maintaining it in a good shape, start automatically testing it
but at the same time avoid exposing incomplete and/or undocumented feature to
end users just yet.
And the docs says that even if the extension proposal PR is merged into main, __DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS macro must be kept. It includes situation when the extension proposal PR is not merged yet. And we will remove this macro only after the spec reaches sycl-2020 branch.
But we have a problem. The process says that
The macro is **not** intended to be used by end users and its purpose is to
simplify our development process by allowing us to merge implementation (full
or partial) of the aforementioned extensions earlier to simplify maintenance and
enable automated testing.
So our end-users cannot and must not use this KHR extension if __DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS still here. That could be a problem for our end users.
So this extension should be finalized in Khronos first.
|
|
||
| #include <type_traits> | ||
|
|
||
| #define SYCL_KHR_PROPERTIES 1 |
There was a problem hiding this comment.
Can you please move that to sycl/source/feature_test.hpp.in under #ifdef __DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS macro?
b68563c to
32bffad
Compare
Exact property-list sizes are ABI-specific (MSVC pads multiple bases sharing a common empty base differently), so restrict the size checks to the Itanium ABI (REQUIRES: linux) and move the portable trivial-copyability checks into properties.cpp.
a0a5e0d to
c9bdcca
Compare
|
@intel/llvm-gatekeepers can you merge this in. |
This is partial implementation of KhronosGroup/SYCL-Docs#980