[SYCL] Remove is_property_value/is_property_value_of

[aelovikov-intel]

The only non-test use was in syclcompat/traits.hpp and we can use detail::AllPropertyValues that doesn't require each property to specialize the trait.

[Pennycook]

Specification changes look good to me. Thanks for all your work on making compile-time properties so much simpler.

[gmlueck]

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

[aelovikov-intel]

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.

[gmlueck]

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.

[Pennycook]

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.

[gmlueck]

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.

[Pennycook]

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.

[aelovikov-intel]

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>;

[aelovikov-intel]

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.

[Pennycook]

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.

[aelovikov-intel]

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

[aelovikov-intel]

@gmlueck , do you think this removal without deprecation is ok? Arguments in favor: unlikely to be used by customers, the feature is experimental. Against: it's possible to change all property_values to provide key_t type-alias and implement generic deprecated is_property_value* so technically we can issue a deprecation.

[aelovikov-intel]

@intel/syclcompat-lib-reviewers , ping

[joeatodd]

syclcompat changes LGTM.

[aelovikov-intel]

@gmlueck , do you think this removal without deprecation is ok? Arguments in favor: unlikely to be used by customers, the feature is experimental. Against: it's possible to change all property_values to provide key_t type-alias and implement generic deprecated is_property_value* so technically we can issue a deprecation.

ping @gmlueck