Repository navigation
[SYCL] Add sycl_khr_properties queue properties - #23133
Conversation
khr/properties.hpp defined its internals in sycl::khr::detail. Because sibling headers under namespace sycl::khr (e.g. free_function_commands.hpp) use bare detail::Foo intending sycl::detail::Foo, unqualified lookup finds sycl::khr::detail first and fails -- so <sycl/sycl.hpp> under __DPCPP_ENABLE_UNFINISHED_KHR_EXTENSIONS did not compile. Move the property-definition infrastructure to sycl::khr::property::detail so no sycl::khr::detail exists. Also fix is_property to exclude the properties list itself (it privately inherits its properties, so is_base_of would report it as a property and is_property_for would probe an ambiguous __detail_key_t). Add a regression test that includes <sycl/sycl.hpp> with the unfinished KHR extensions enabled.
Add the queue properties khr::property::{enable_profiling,in_order} (runtime
bool, separate keys) to the single khr/properties.hpp, and the queue
constructor overloads accepting a khr property or property list plus
queue::khr_is_profiling_enabled(). The constructors translate the runtime-valued
khr properties into an old-style property_list via a new befriended
detail::PropertyListBuilder (property_list's runtime bitset constructor).
is_in_order() already existed.
MSVC rejects the static (compile-time key) and non-static const (runtime key) get_property overloads as differing only in return type during class definition, before the enable_if SFINAE applies. Give the runtime overload a defaulted dummy int parameter so the two have distinct parameter lists (as the experimental properties container does).
|
please don't forget to add link to the spec for these changes. |
KseniyaTikhomirova
left a comment
KseniyaTikhomirova
left a comment
There was a problem hiding this comment.
LGTM in general
|
|
||
| namespace sycl { | ||
| inline namespace _V1 { | ||
| // Forward declarations of the SYCL classes that the named properties below |
There was a problem hiding this comment.
please consider removal of AI generated comments that doesn't bring value to the code. applicable to many comments
| // True if T is a khr::properties list. Used to exclude the list itself from | ||
| // is_property (the list privately inherits its properties, so is_base_of would | ||
| // otherwise report it as a property). | ||
| template <typename> struct is_properties_list : std::false_type {}; |
There was a problem hiding this comment.
this file has sections splitted up comments like "Property tags" and "Property traits". this struct is added to the wrong one. I suppose it is done due to namespace scope. Probably then consider removal of sections/comments that you can't keep relevant
| // it to the corresponding old-style property_list. PropertyOrList is not | ||
| // defaulted: the property_list constructors already cover the no-property | ||
| // case, and defaulting here would make e.g. `queue q{}` ambiguous. | ||
| private: |
There was a problem hiding this comment.
could you move it to the private section below please? this approach brings a mess to the class declaration
There was a problem hiding this comment.
this is not done, I still think it would be nice to keep public API methods first - aligned with other classes. No need to start class declaration with helpers.
I fixed and restructured the code to make it cleaner, and trimmed comments. |
| // (C2686) during class definition, before the enable_if SFINAE applies. | ||
| template <typename PropertyKey> | ||
| constexpr auto get_property() const -> std::enable_if_t< | ||
| constexpr auto get_property(int = 0) const -> std::enable_if_t< |
There was a problem hiding this comment.
@gmlueck we have 2 functions in the spec with the same name + signature. Do you think we should rename one of them in the spec?
There was a problem hiding this comment.
I think the oneapi extension is the same way -- we have two overloads of the same member function with mutually exclusive constraints. Do we have the same problem with MSVC in the current extension?
There was a problem hiding this comment.
Yes, the oneapi extension has the same static/non-static pair and already carries the workaround. The runtime overload is get_property(int = 0) const. That dummy int is the mitigation; without it MSVC rejects it the same way.
The trigger isn't the mutually-exclusive constraints. SFINAE overloads are fine. It's that one overload is static and the other non-static with an otherwise-identical parameter-type-list.
There was a problem hiding this comment.
Using the same function name for both compile-time properties and runtime properties seems nice from a user standpoint, so I think that outweighs the ugly implementation on MSVC. @rolandschulz do you have an opinion here?
|
@intel/llvm-gatekeepers This is ready to be merged.
|
Implements the queue portion of sycl_khr_properties: the properties
khr::property::enable_profilingandkhr::property::in_order, the queueconstructor overloads accepting a khr property or property list, and
queue::khr_is_profiling_enabled()(is_in_order()already existed). Thenew-style properties are translated to the existing
property::queue::*.KHR Link: KhronosGroup/SYCL-Docs#980