Skip to content

[SYCL] Remove is_property_value/is_property_value_of #15899

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

Closed
wants to merge 1 commit into from
Closed

[SYCL] Remove is_property_value/is_property_value_of #15899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 4 additions & 17 deletions sycl/doc/extensions/experimental/sycl_ext_oneapi_properties.asciidoc
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw you made this comment in an old PR. I assume your concern is related to this new property work that you're doing, so let's discuss it here.

I would like to change is_property_key_of (is_property_of) into a more general trait that works even for properties that are not used in a constructor. For example, some of the free functions related to the kernel compiler take properties, and I'd like to use is_property_of to indicate which properties are allowed.

My thought was to define a new class name something like this:

namespace sycl::ext::oneapi::experimental {

struct build_source_bundle_properties;

}

and then allow application code to test whether a property is allowed for this function via is_property_of_v<Prop, build_source_bundle_properties>.

I think the same strategy could be used in #14743 to define a trait that can be used to tell which properties are allowed in a launch_config.

Copy link
Contributor

Choose a reason for hiding this comment

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

If build_source_bundle_properties is just a property list (type), do we need a trait at all? Could we just use build_source_bundle_properties.has_property<PropertyT>() to check whether a given property exists in some list of valid properties?

We could then extend this to classes by defining something like:

template <typename T, ...>
struct buffer {
  using valid_properties = decltype(sycl::properties{ /* whatever the valid properties are */ });
};

We wouldn't need to define is_property_of if we took this approach, but we could define it as a shorthand in terms of this valid_properties member if we thought it was important.

One consequence of this approach is that somebody wouldn't be able to add new properties to classes that aren't already expecting them. I don't know whether that's an advantage or a disadvantage.

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposed build_source_bundle_properties is not a property list. It's just an incomplete type, defined exactly as I show above in my comment. We just use this type as a sort of "handle" that can be used to identify a particular set of properties.

The valid_properties approach you suggest only works for properties that are associated with some class (because valid_properties is a class member). In the example I illustrate above with the kernel compiler, there is no relevant class. Instead, we want to know which properties are allowed to be used with a particular overload of the build fiunction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I wasn't clear: I'm suggesting that instead of build_source_bundle_properties being an incomplete type, it could be defined as an alias to a property list type. We could still define multiple such aliases to cover multiple free functions. The difference in what I was proposing is that you'd be able to use members of the property list class to query what the list of valid properties contains, instead of having to create new traits.

I only mentioned classes to show that defining a list of valid properties would also cover that case.

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's better to have this discussion in #15752. What I'm trying to do is to use #15752 to determine our final goal and then have a bunch of trivial incremental changes that would eventually lead there. My current plan after this removal is to eliminate template <...> class property_value and (possibly in a separate PR) introduce property_base. After that I'll move LLVM IR attribute metadata from a separate class directly to properties.

As for this discussion, I tend to agree with @Pennycook. What I was envisioning before my last change there (eliminating property key type from the user interfaces) is

llvm/sycl/include/sycl/ext/oneapi/properties/new_properties.hpp

Lines 354 to 368 in 07ac818

// FIXME:
template <typename property_list_ty, typename... allowed_property_keys>
struct all_properties_in : std::false_type {};
template <typename... property_tys, typename... allowed_property_keys>
struct all_properties_in<
properties<detail::properties_type_list<property_tys...>>,
allowed_property_keys...>
: std::bool_constant<((sycl::detail::check_type_in_v<
property_tys, allowed_property_keys...> &&
...))> {};
template <typename property_list_ty, typename... allowed_property_keys>
inline constexpr bool all_properties_in_v =
all_properties_in<std::remove_const_t<property_list_ty>,
allowed_property_keys...>::value;
,

then each interface (either class or free function) could just do SFINAE/static_assert over all_properties_in_v<PropListTy, foo_key, bar_key, baz_key>. One can alias it if it's used in multiple places:

template <typename PropListTy>
inline constexpr bool verify_properties_for_xyz_interface = 
    all_properties_in_v<PropListTy, foo_key, bar_key, baz_key>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm suggesting that instead of build_source_bundle_properties being an incomplete type, it could be defined as an alias to a property list type.

I don't like this, because property list is a collection of instances of properties (values) and not a collection of properties (types and/or templates). Also, having "conflicting" properties in a single property list feels hacky to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this, because property list is a collection of instances of properties (values) and not a collection of properties (types and/or templates). Also, having "conflicting" properties in a single property list feels hacky to me.

Oh, I hadn't considered that. You're right, constructing an invalid property list just for the sake of describing the list of possible properties would be a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest to continue this sub-thread in https://github.com/intel/llvm/pull/15752/files#r1821560396

Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,10 @@ inline constexpr boo_key::value_t<Ts...> boo;
=== Property traits

All runtime and compile-time-constant properties must have a specialization of
`is_property_key` and `is_property_value` that inherits from
`std::true_type`, and they must have a specialization of `is_property_key_of`
and `is_property_value_of`
that inherits from `std::true_type` for each SYCL runtime class that the
property can be applied to. All have a base case which inherits from `std::false_type`.
`is_property_key` that inherits from `std::true_type`, and they must have a
specialization of `is_property_key_of` that inherits from `std::true_type` for
each SYCL runtime class that the property can be applied to. All have a base
case which inherits from `std::false_type`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't have separate traits for "key" and "value", should we rename these traits to is_property and is_property_of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it would be wise. The remaining traits (after this PR) still apply to the keys, while future is_property would reference the property/value and the keys will be hidden as implementation details.


```c++
namespace sycl::ext::oneapi::experimental {
Expand All @@ -221,18 +220,6 @@ struct is_property_key_of<foo_key, SyclObjectT> : std::true_type {};
template<typename SyclObjectT>
struct is_property_key_of<bar_key, SyclObjectT> : std::true_type {};

// is_property_value and is_property_value_of based on is_property_key(_of)
template<typename V, typename=void> struct is_property_value;
template<typename V, typename O, typename=void> struct is_property_value_of;
// Specialization for runtime properties
template<typename V> struct is_property_value<V, std::enable_if_t<(sizeof(V)>0)>> : is_property_key<V> {};
template<typename V, typename O> struct is_property_value_of<V, O, std::enable_if_t<(sizeof(V)>0)>> : is_property_key_of<V,O> {};
// Specialization for compile-time-constant properties
template<typename V> struct is_property_value<V, std::void_t<typename V::key_t>> :
is_property_key<typename V::key_t> {};
template<typename V, typename O> struct is_property_value_of<V, O, std::void_t<typename V::key_t>> :
is_property_key_of<typename V::key_t, O> {};

} // namespace experimental::oneapi::ext::sycl
```

Expand Down
22 changes: 0 additions & 22 deletions sycl/include/sycl/ext/oneapi/experimental/cluster_group_prop.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,28 +47,6 @@ struct is_property_key_of<cuda::cluster_size_key<2>, T> : std::true_type {};
template <typename T>
struct is_property_key_of<cuda::cluster_size_key<3>, T> : std::true_type {};

template <>
struct is_property_value<cuda::cluster_size_key<1>>
: is_property_key<cuda::cluster_size_key<1>> {};
template <>
struct is_property_value<cuda::cluster_size_key<2>>
: is_property_key<cuda::cluster_size_key<2>> {};
template <>
struct is_property_value<cuda::cluster_size_key<3>>
: is_property_key<cuda::cluster_size_key<3>> {};

template <typename O>
struct is_property_value_of<cuda::cluster_size_key<1>, O>
: is_property_key_of<cuda::cluster_size_key<1>, O> {};

template <typename O>
struct is_property_value_of<cuda::cluster_size_key<2>, O>
: is_property_key_of<cuda::cluster_size_key<2>, O> {};

template <typename O>
struct is_property_value_of<cuda::cluster_size_key<3>, O>
: is_property_key_of<cuda::cluster_size_key<3>, O> {};

namespace detail {
template <typename PropertiesT> constexpr std::size_t getClusterDim() {
if constexpr (PropertiesT::template has_property<
Expand Down
16 changes: 0 additions & 16 deletions sycl/include/sycl/ext/oneapi/properties/property_value.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,6 @@ operator!=(const property_value<PropertyT, A...> &,
return (!std::is_same<A, B>::value || ...);
}

template <typename V, typename = void> struct is_property_value {
static constexpr bool value =
detail::IsRuntimeProperty<V>::value && is_property_key<V>::value;
};
template <typename V, typename O, typename = void> struct is_property_value_of {
static constexpr bool value =
detail::IsRuntimeProperty<V>::value && is_property_key_of<V, O>::value;
};
// Specialization for compile-time-constant properties
template <typename V>
struct is_property_value<V, std::void_t<typename V::key_t>>
: is_property_key<typename V::key_t> {};
template <typename V, typename O>
struct is_property_value_of<V, O, std::void_t<typename V::key_t>>
: is_property_key_of<typename V::key_t, O> {};

namespace detail {

// Specialization of PropertyID for propagating IDs through property_value.
Expand Down
5 changes: 3 additions & 2 deletions sycl/include/syclcompat/traits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,9 @@ inline constexpr bool is_launch_policy_v = is_launch_policy<T>::value;

// Trait to detect if all args are sycl_exp property types
template <typename... Args>
using are_all_props = std::conjunction<
sycl::ext::oneapi::experimental::is_property_value<Args>...>;
using are_all_props =
sycl::ext::oneapi::experimental::detail::AllPropertyValues<
std::tuple<Args...>>;

} // namespace experimental::detail

Expand Down
15 changes: 1 addition & 14 deletions sycl/test/extensions/annotated_arg/annotated_arg_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ static annotated_arg<int *, decltype(properties(buffer_location<1>,

struct A {};

// Checks is_property_key_of and is_property_value_of for T.
// Checks is_property_key_of for T.
template <typename T> void checkIsPropertyOf() {
static_assert(is_property_key_of<register_map_key, T>::value);
static_assert(is_property_key_of<conduit_key, T>::value);
Expand All @@ -31,19 +31,6 @@ template <typename T> void checkIsPropertyOf() {
static_assert(is_property_key_of<read_write_mode_key, T>::value);
static_assert(is_property_key_of<maxburst_key, T>::value);
static_assert(is_property_key_of<wait_request_key, T>::value);

static_assert(is_property_value_of<decltype(register_map), T>::value);
static_assert(is_property_value_of<decltype(conduit), T>::value);
static_assert(is_property_value_of<decltype(stable), T>::value);

static_assert(is_property_value_of<decltype(buffer_location<1>), T>::value);
static_assert(is_property_value_of<decltype(awidth<2>), T>::value);
static_assert(is_property_value_of<decltype(dwidth<8>), T>::value);
static_assert(is_property_value_of<decltype(latency<0>), T>::value);
static_assert(is_property_value_of<decltype(read_write_mode_read), T>::value);
static_assert(is_property_value_of<decltype(maxburst<1>), T>::value);
static_assert(
is_property_value_of<decltype(wait_request_requested), T>::value);
}

int main() {
Expand Down
18 changes: 2 additions & 16 deletions sycl/test/extensions/annotated_ptr/annotated_ptr_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ static annotated_ptr<int, decltype(properties(buffer_location<1>,

struct A {};

// Checks is_property_key_of and is_property_value_of for T.
// Checks is_property_key_of for T.
template <typename T> void checkIsPropertyOf() {
static_assert(is_property_key_of<register_map_key, T>::value);
static_assert(is_property_key_of<conduit_key, T>::value);
Expand All @@ -29,23 +29,9 @@ template <typename T> void checkIsPropertyOf() {
static_assert(is_property_key_of<read_write_mode_key, T>::value);
static_assert(is_property_key_of<maxburst_key, T>::value);
static_assert(is_property_key_of<wait_request_key, T>::value);

static_assert(is_property_value_of<decltype(register_map), T>::value);
static_assert(is_property_value_of<decltype(conduit), T>::value);
static_assert(is_property_value_of<decltype(stable), T>::value);

static_assert(is_property_value_of<decltype(buffer_location<1>), T>::value);
static_assert(is_property_value_of<decltype(awidth<2>), T>::value);
static_assert(is_property_value_of<decltype(dwidth<8>), T>::value);
static_assert(is_property_value_of<decltype(latency<0>), T>::value);
static_assert(is_property_value_of<decltype(read_write_mode_read), T>::value);
static_assert(is_property_value_of<decltype(maxburst<1>), T>::value);
static_assert(
is_property_value_of<decltype(wait_request_requested), T>::value);
}

// Checks is_property_key_of and is_property_value_of are false for non-pointer
// type T.
// Checks is_property_key_of is false for non-pointer type T.
template <typename T> void checkIsValidPropertyOfNonPtr() {
static_assert(
is_valid_property<T, decltype(wait_request_not_requested)>::value ==
Expand Down
23 changes: 1 addition & 22 deletions sycl/test/extensions/device_global/device_global_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,12 @@ static device_global<int, decltype(properties(
device_image_scope, init_mode_reprogram))>
DeviceGlobal5;

// Checks is_property_key_of and is_property_value_of for T.
// Checks is_property_key_of for T.
template <typename T> void checkIsPropertyOf() {
static_assert(is_property_key_of<device_image_scope_key, T>::value);
static_assert(is_property_key_of<host_access_key, T>::value);
static_assert(is_property_key_of<init_mode_key, T>::value);
static_assert(is_property_key_of<implement_in_csr_key, T>::value);

static_assert(is_property_value_of<decltype(device_image_scope), T>::value);
static_assert(is_property_value_of<decltype(host_access_read), T>::value);
static_assert(is_property_value_of<decltype(host_access_write), T>::value);
static_assert(
is_property_value_of<decltype(host_access_read_write), T>::value);
static_assert(is_property_value_of<decltype(host_access_none), T>::value);
static_assert(is_property_value_of<decltype(init_mode_reset), T>::value);
static_assert(is_property_value_of<decltype(init_mode_reprogram), T>::value);
static_assert(is_property_value_of<decltype(implement_in_csr_on), T>::value);
static_assert(is_property_value_of<decltype(implement_in_csr_off), T>::value);
}

int main() {
Expand All @@ -50,16 +39,6 @@ int main() {
static_assert(is_property_key<init_mode_key>::value);
static_assert(is_property_key<implement_in_csr_key>::value);

static_assert(is_property_value<decltype(device_image_scope)>::value);
static_assert(is_property_value<decltype(host_access_read)>::value);
static_assert(is_property_value<decltype(host_access_write)>::value);
static_assert(is_property_value<decltype(host_access_read_write)>::value);
static_assert(is_property_value<decltype(host_access_none)>::value);
static_assert(is_property_value<decltype(init_mode_reset)>::value);
static_assert(is_property_value<decltype(init_mode_reprogram)>::value);
static_assert(is_property_value<decltype(implement_in_csr_on)>::value);
static_assert(is_property_value<decltype(implement_in_csr_off)>::value);

checkIsPropertyOf<decltype(DeviceGlobal1)>();
static_assert(DeviceGlobal1.has_property<device_image_scope_key>());
static_assert(!DeviceGlobal1.has_property<host_access_key>());
Expand Down
30 changes: 1 addition & 29 deletions sycl/test/extensions/fpga_mem/fpga_mem_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ static intel::fpga_mem<int, decltype(oneapi::properties(
intel::bi_directional_ports_true))>
mem_multi;

// Checks is_property_key_of and oneapi::is_property_value_of for T.
// Checks is_property_key_of for T.
template <typename T> void checkIsPropertyOf() {
static_assert(oneapi::is_property_key_of<intel::resource_key, T>::value);
static_assert(oneapi::is_property_key_of<intel::num_banks_key, T>::value);
Expand Down Expand Up @@ -51,34 +51,6 @@ int main() {
static_assert(oneapi::is_property_key<intel::max_private_copies_key>::value);
static_assert(oneapi::is_property_key<intel::num_replicates_key>::value);

// Are all common values usable
static_assert(
oneapi::is_property_value<decltype(intel::resource_mlab)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::resource_block_ram)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::num_banks<8>)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::stride_size<8>)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::word_size<32>)>::value);
static_assert(oneapi::is_property_value<
decltype(intel::bi_directional_ports_false)>::value);
static_assert(oneapi::is_property_value<
decltype(intel::bi_directional_ports_true)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::clock_2x_false)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::clock_2x_true)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::ram_stitching_min_ram)>::value);
static_assert(oneapi::is_property_value<
decltype(intel::ram_stitching_max_fmax)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::max_private_copies<8>)>::value);
static_assert(
oneapi::is_property_value<decltype(intel::num_replicates<8>)>::value);

// Check that only the property that are expected are associated with obj
checkIsPropertyOf<decltype(mem_num)>();
static_assert(mem_num.has_property<intel::num_banks_key>());
Expand Down
14 changes: 0 additions & 14 deletions sycl/test/extensions/properties/properties_device_global.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,6 @@ int main() {
static_assert(is_property_key<init_mode_key>::value);
static_assert(is_property_key<implement_in_csr_key>::value);

// Check that is_property_value is correctly specialized.
static_assert(is_property_value<decltype(device_image_scope)>::value);
static_assert(is_property_value<decltype(host_access<TestAccess>)>::value);
static_assert(is_property_value<decltype(host_access_read)>::value);
static_assert(is_property_value<decltype(host_access_write)>::value);
static_assert(is_property_value<decltype(host_access_read_write)>::value);
static_assert(is_property_value<decltype(host_access_none)>::value);
static_assert(is_property_value<decltype(init_mode<TestTrigger>)>::value);
static_assert(is_property_value<decltype(init_mode_reprogram)>::value);
static_assert(is_property_value<decltype(init_mode_reset)>::value);
static_assert(is_property_value<decltype(implement_in_csr<true>)>::value);
static_assert(is_property_value<decltype(implement_in_csr_on)>::value);
static_assert(is_property_value<decltype(implement_in_csr_off)>::value);

// Checks that fully specialized properties are the same as the templated
// variants.
static_assert(std::is_same_v<decltype(host_access_read),
Expand Down
90 changes: 0 additions & 90 deletions sycl/test/extensions/properties/properties_is_property_value.cpp
View file
Open in desktop

This file was deleted.

Loading
Loading