Skip to content

[SYCL] Add sycl_khr_properties queue properties #23133

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
uditagarwal97 merged 4 commits into from
Sep 15, 2026
Merged

[SYCL] Add sycl_khr_properties queue properties #23133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7400cb2
[SYCL] Fix sycl::khr::detail shadowing sycl::detail
koparasy Sep 9, 2026
66f04b2
[SYCL] Add sycl_khr_properties queue properties
koparasy Sep 9, 2026
48cf7ba
[SYCL] Fix MSVC C2686 on khr::properties::get_property
koparasy Sep 10, 2026
b46e2aa
Address reviews
koparasy Sep 14, 2026
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
151 changes: 82 additions & 69 deletions sycl/include/sycl/khr/properties.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,10 @@
//===----------------------------------------------------------------------===//
//
// Implementation of the sycl_khr_properties extension: the
// `sycl::khr::properties` compile-time property list container and the
// associated classification traits.
// `sycl::khr::properties` container and its classification traits.
//
// Design notes:
// * The list stores its properties as private base classes. Properties whose
// values are all compile-time (empty types) are NOT stored -- they are
// default-constructed on retrieval -- so an all-compile-time list is
// zero-overhead and a mixed list only pays for its runtime members. This
// keeps the list trivially copyable and cheap to compile (no std::tuple).
// * The list does not canonicalize (sort) its element order. Per the
// extension, two lists built from the same properties in a different order
// may have different types; comparison operators are intentionally not
// provided.
// Compile-time-only properties are not stored (they are default-constructed on
// retrieval); the list preserves element order (no sorting) per the extension.
//
//===----------------------------------------------------------------------===//

Expand All @@ -34,22 +25,24 @@

namespace sycl {
inline namespace _V1 {
// Forward declarations of the SYCL classes that the named properties below

@KseniyaTikhomirova KseniyaTikhomirova Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please consider removal of AI generated comments that doesn't bring value to the code. applicable to many comments

// register against (via is_property_key_for). Only incomplete types are needed.
class queue;

namespace khr {

template <typename... EncodedProperties> class __SYCL_EBO properties;

// The property-definition infrastructure lives in `sycl::khr::property::detail`
// (not `sycl::khr::detail`) so it does not shadow `sycl::detail` for
// unqualified `detail::` lookups in sibling headers under `namespace
// sycl::khr`.
namespace property {
namespace detail {

//===----------------------------------------------------------------------===//
// Base tags
//
// Every property derives (directly or via a convenience base) from
// `property_tag`, and exposes a `__detail_key_t` alias naming its key. Every
// key derives from `property_key_tag`; a key whose property has no
// runtime-provided values additionally derives from
// `compile_time_property_key_tag`.
//===----------------------------------------------------------------------===//

// Every property derives from `property_tag` and exposes a `__detail_key_t`
// alias naming its key. Every key derives from `property_key_tag`; a
// compile-time key also derives from `compile_time_property_key_tag`.
struct property_tag {};
struct property_key_tag {};
struct compile_time_property_key_tag : property_key_tag {};
Expand All @@ -67,55 +60,36 @@ inline constexpr bool __detail_has_runtime_value =
!std::is_base_of_v<compile_time_property_key_tag,
typename Prop::__detail_key_t>;

//===----------------------------------------------------------------------===//
// Convenience base classes for defining properties
//
// These mirror the patterns in the extension's "Examples for implementors".
// Implementations may define properties directly on `property_base`/the tags,
// but these reduce boilerplate for the common shapes.
//===----------------------------------------------------------------------===//
// Convenience base classes for defining properties, mirroring the extension's
// examples for implementors. Properties may also be defined directly on the
// tags above.

// Base for a runtime property key (all of the property's values are supplied at
// runtime). Usage:
// struct my_key : detail::runtime_property_key {};
// struct my_prop : detail::runtime_property<my_key> { int value; ... };
// Runtime property key: all of the property's values are supplied at runtime.
struct runtime_property_key : property_key_tag {};
template <typename Key> struct runtime_property : property_base<Key> {};

// Base for a compile-time property key with a single non-type value. Usage:
// struct my_key : detail::constant_value_property_key {};
// template <int V>
// inline constexpr my_key::__detail_property_t<my_key, int, V> my_prop;
// Compile-time key with a single non-type value.
struct constant_value_property_key : compile_time_property_key_tag {
template <typename PropertyKey, typename Type, Type Value>
struct __detail_property_t : property_base<PropertyKey> {
static constexpr Type value = Value;
};
};

// Base for a compile-time property key with a single type value. Usage:
// struct my_key : detail::constant_type_property_key {};
// template <typename T>
// inline constexpr my_key::__detail_property_t<my_key, T> my_prop;
// Compile-time key with a single type value.
struct constant_type_property_key : compile_time_property_key_tag {
template <typename PropertyKey, typename Type>
struct __detail_property_t : property_base<PropertyKey> {
using value_t = Type;
};
};

// Base for a hybrid property key (some values compile-time, some runtime). The
// key is a runtime key (the property carries runtime data and is stored).
// Usage:
// struct my_key : detail::hybrid_property_key {};
// template <int X> struct my_prop : detail::hybrid_property<my_key> {
// static constexpr int x = X; int y; constexpr my_prop(int y):y{y}{} };
// Hybrid property key: some values compile-time, some runtime. The property
// carries runtime data and is stored.
struct hybrid_property_key : property_key_tag {};
template <typename Key> struct hybrid_property : property_base<Key> {};

//===----------------------------------------------------------------------===//
// Retrieval / storage machinery
//===----------------------------------------------------------------------===//
// Retrieval / storage machinery.

// Selects, from a pack of properties, the one whose key is `Key`. Assumes
// exactly one match (enforced by the "no duplicate key" mandate).
Expand Down Expand Up @@ -162,25 +136,36 @@ struct build_storage<property_storage<Sel...>, P, Rest...>
template <typename... All>
using storage_for = typename build_storage<property_storage<>, All...>::type;

// 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 {};

@KseniyaTikhomirova KseniyaTikhomirova Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@koparasy koparasy Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cleaned it up.

template <typename... Ps>
struct is_properties_list<properties<Ps...>> : std::true_type {};

} // namespace detail
} // namespace property

//===----------------------------------------------------------------------===//
// Property traits
//===----------------------------------------------------------------------===//
// Classification traits.

// A property list is not itself a property: it privately inherits its
// properties, so is_base_of would otherwise report it as one -- exclude it.
template <typename T>
struct is_property : std::is_base_of<detail::property_tag, T> {};
struct is_property
: std::bool_constant<std::is_base_of_v<property::detail::property_tag, T> &&
Comment thread
KseniyaTikhomirova marked this conversation as resolved.
Resolved
!property::detail::is_properties_list<T>::value> {};
template <typename T>
inline constexpr bool is_property_v = is_property<T>::value;

template <typename T>
struct is_property_key : std::is_base_of<detail::property_key_tag, T> {};
struct is_property_key
: std::is_base_of<property::detail::property_key_tag, T> {};
template <typename T>
inline constexpr bool is_property_key_v = is_property_key<T>::value;

template <typename T>
struct is_property_key_compile_time
: std::is_base_of<detail::compile_time_property_key_tag, T> {};
: std::is_base_of<property::detail::compile_time_property_key_tag, T> {};
template <typename T>
inline constexpr bool is_property_key_compile_time_v =
is_property_key_compile_time<T>::value;
Expand Down Expand Up @@ -213,14 +198,11 @@ template <typename T, typename Class>
inline constexpr bool is_property_list_for_v =
is_property_list_for<T, Class>::value;

//===----------------------------------------------------------------------===//
// The properties class
//===----------------------------------------------------------------------===//

// The properties container.
template <typename... EncodedProperties>
class __SYCL_EBO properties
: private detail::storage_for<EncodedProperties...> {
using storage_t = detail::storage_for<EncodedProperties...>;
: private property::detail::storage_for<EncodedProperties...> {
using storage_t = property::detail::storage_for<EncodedProperties...>;

static_assert((is_property_v<EncodedProperties> && ...),
"Template arguments of khr::properties must be properties.");
Expand Down Expand Up @@ -251,18 +233,23 @@ class __SYCL_EBO properties
template <typename PropertyKey>
static constexpr auto get_property() -> std::enable_if_t<
is_property_key_compile_time_v<PropertyKey>,
detail::property_of_key_t<PropertyKey, EncodedProperties...>> {
return detail::property_of_key_t<PropertyKey, EncodedProperties...>{};
property::detail::property_of_key_t<PropertyKey, EncodedProperties...>> {
return property::detail::property_of_key_t<PropertyKey,
EncodedProperties...>{};
}

// Runtime (or hybrid) key: return a copy of the stored property.
//
// The dummy `int` parameter gives this overload a parameter list distinct
// from the static compile-time overload above. Without it, MSVC rejects the
// two overloads (static vs non-static) as differing only in return type
// (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<

@koparasy koparasy Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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?

@gmlueck gmlueck Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@koparasy koparasy Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@gmlueck gmlueck Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

!is_property_key_compile_time_v<PropertyKey>,
detail::property_of_key_t<PropertyKey, EncodedProperties...>> {
return static_cast<
const detail::property_of_key_t<PropertyKey, EncodedProperties...> &>(
*this);
property::detail::property_of_key_t<PropertyKey, EncodedProperties...>> {
return static_cast<const property::detail::property_of_key_t<
PropertyKey, EncodedProperties...> &>(*this);
}
};

Expand All @@ -273,6 +260,32 @@ properties(Properties... props) -> properties<Properties...>;

using empty_properties_t = decltype(properties{});

// Named properties. Each registers its supported classes via
// is_property_key_for; constructor support lives in the object headers (e.g.
// queue.hpp).
namespace property {
namespace key {
struct enable_profiling : detail::runtime_property_key {};
struct in_order : detail::runtime_property_key {};
} // namespace key

// Queue properties.
struct enable_profiling : detail::runtime_property<key::enable_profiling> {
constexpr enable_profiling(bool v = true) : value{v} {}
bool value;
};
struct in_order : detail::runtime_property<key::in_order> {
constexpr in_order(bool v = true) : value{v} {}
bool value;
};
} // namespace property

template <>
struct is_property_key_for<property::key::enable_profiling, queue>
: std::true_type {};
template <>
struct is_property_key_for<property::key::in_order, queue> : std::true_type {};

} // namespace khr
} // namespace _V1
} // namespace sycl
Expand Down
20 changes: 20 additions & 0 deletions sycl/include/sycl/property_list.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ template <typename... PropsT> class accessor_property_list;
namespace detail {
class PropertyValidator;
class SYCLMemObjT;
class PropertyListBuilder;
} // namespace detail

/// Objects of the property_list class are containers for the SYCL properties
Expand Down Expand Up @@ -71,6 +72,7 @@ class property_list : protected detail::PropertyListBase {
friend class ext::oneapi::accessor_property_list;
friend class detail::PropertyValidator;
friend class detail::SYCLMemObjT;
friend class detail::PropertyListBuilder;
};

namespace detail {
Expand All @@ -83,6 +85,24 @@ class PropertyValidator {
std::move(FunctionForData));
}
};

// Builds a property_list at runtime. Used to translate runtime-valued
// sycl_khr_properties into an old-style property_list.
class PropertyListBuilder {
std::bitset<DataLessPropKind::DataLessPropKindSize> MDataLessProps;
std::vector<std::shared_ptr<PropertyWithDataBase>> MPropsWithData;

public:
template <typename DataLessPropT> void add() {
MDataLessProps.set(DataLessPropT::getKind());
}
void add(std::shared_ptr<PropertyWithDataBase> Prop) {
MPropsWithData.push_back(std::move(Prop));
}
property_list finalize() {
return property_list(MDataLessProps, MPropsWithData);
}
};
} // namespace detail

} // namespace _V1
Expand Down
Loading
Loading