[oneDPL] Indirectly Device Accessible Iterator Customization Point and Public Trait

[danhoeflinger]

Specification for "indirectly device accessible iterator" customization point and public trait. Based upon proposed [RFC].(uxlfoundation/oneDPL#1999)

[rarutyun]

Despite a significant amount of suggestions, I think that wording is pretty good, overall. There is one place that is related to the design, where we need to be careful to not prohibit some intendent use-cases.

[rarutyun]

I think it's not true for a generic case. As I mentioned, I believe std::conjunction is an important use case. However, std::conjunction might not have std::true_type or std::false_type characteristics at all.

I believe that this sentence about characteristics has a perfect fit to where you describe is_device_accessible_content_iterator trait itself instead of the current wording, which is "... evaluates to a type...". I would just say "...has characteristics of std::true_type ..."

Here we need something else. Don't have immediate suggestions, though

[akukanov]

However, std::conjunction might not have std::true_type or std::false_type characteristics at all.

If it is used with true_type/false_type or derivatives, it will:

The specialization std::conjunction<B1, ..., BN> has a public and unambiguous base that is

• if sizeof...(B) == 0, std::true_type;
• otherwise the first type Bi in B1, ..., BN for which bool(Bi::value) == false, or BN if there is no such type.

And that's exactly our goal I believe.

[danhoeflinger]

I suppose I don't know exactly what we mean by "characteristics of std::true_type" if a std::conjunction or similar types will not qualify.

[rarutyun]

conjunction, disjunction and possibly negation (though, I am not sure) are the only exceptions I am aware of. All other traits have characteristics of either true_type or false_type because they inherit from bool_constant

[akukanov]

I suppose I don't know exactly what we mean by "characteristics of std::true_type" if a std::conjunction or similar types will not qualify.

The wording "with a base characteristic of" is used in a few places in the C++ standard, for example in https://eel.is/c++draft/meta.unary:

Subclause [meta.unary] contains templates that may be used to query the properties of a type at compile time.
Each of these templates shall be a Cpp17UnaryTypeTrait with a base characteristic of true_type if the corresponding condition is true, otherwise false_type.

My understanding of this wording is: even though it is not a true_type or false_type, it has all the same characteristics as these types and can be used as such. Practically it means that the trait class inherits either true_type or false_type (maybe indirectly).

In my comment above, I say that std::conjunction also falls into this category if the chosen template parameter does. Though the standard does not prohibit to use other types with std::conjunction, typical arguments it is used with will be traits etc. classes that inherit bool_constant. Therefore requiring that the return type of our ADL customization function should have these characteristics does not preclude the use of std::conjunction, but limits what types can be passed to it.

[akukanov]

My understanding of this wording is: even though it is not a true_type or false_type, it has all the same characteristics as these types and can be used as such.

Actually, it's even more strict - the standard requires public inheritance, see https://eel.is/c++draft/meta.rqmts#:Cpp17UnaryTypeTrait:

It shall be ... publicly and unambiguously derived, directly or indirectly, from its base characteristic ... The member names of the base characteristic shall not be hidden and shall be unambiguously available ....

[danhoeflinger]

Is see, thanks for the clarification. I think I agree that our current formulation makes sense then, with a small tweak to the language perhaps to follow the example language you provided.

Unless we have a compelling example to allow conjunction disjunction negation which use something other than types with the base characteristic of a bool_constant, I think simplicity is better.

[danhoeflinger]

To follow up here, I've adjusted the language slightly, but think this simple version is best without making it more broad. I've not been able to come up with any real compelling case for conjunction or disjunction of an integral_constant which is not a bool_constant worth making this language more complex.

However, I think a valid implementation could easily accommodate arbitrary usage of conjunction and disjunction, ensuring that the public trait is always a bool_constant.

[rarutyun]

Suggested change

	are SYCL device-copyable, and are always "device accessible content iterators" if both the ``SourceIterator``
	are SYCL device-copyable, and are "device accessible content iterators" if both the ``SourceIterator``

"Always" word seems redundant to me. The same comment is applicable for the iterators below.

[danhoeflinger]

removed all instances of this.

[rarutyun]

This sentence sounds a bit awkward to me. ADL last letter already means lookup. Also, "valid search locations" is a sort of unusual. How about this:

Suggested change

	otherwise. The function must be defined in one of the valid search locations for ADL lookup, which includes the
	namespace of the definition of the iterator type ``IteratorT``.
	otherwise. The function must be discoverable by ADL, which includes the
	namespace of the definition of the iterator type ``IteratorT``.

You may leave this part, if you think it's useful: "... , which includes the
namespace of the definition of the iterator type IteratorT" but it also might be removed, I think. They choice is yours.

[danhoeflinger]

Taken, I removed the second part. It doesn't really make sense after the edit. Its more something for the documentation than the spec.

[rarutyun]

Still don't like this style of writing after a template name because it confuses (probably experienced) C++ users. Is the any chance we will define the API properly?

Something like

The following class template and variable template are defined in namespace oneapi::dpl:

template <typename T>
struct is_device_accessible_content_iterator;

template <typename T>
inline constexpr bool is_device_accessible_content_iterator = is_device_accessible<T>::value;

and then the descriptive part.

[danhoeflinger]

Adjusted to be close to your proposal.

[rarutyun]

naming suggestions to consider

• is_passed_directly_to_device (perhaps obsolete)
• is_device_accessible_iterator
• is_indirectly_device_accessible

For the last one it might be is_device_indirectly_accessible; not sure what is more accurate from English perspective. But the last proposed option (by me) is my favorite, so far. It also corelates with several C++20 concepts, e.g., indirectly_writable

[danhoeflinger]

I don't really like is_device_accessible_iterator because I think there is a difference between the iterator being accessible and its content it refers to being accessible. However, is_indirectly_device_accessible is more clear and concise than what is currently in the PR.

[akukanov]

I am fine with is_indirectly_device_accessible, but we need to get feedback from the customers (that applies to any name(s) we eventually propose).

[rarutyun]

I was also thinking that is_device_accessible_iterator partially reflects the purpose. On the other hand we pass iterators to eventually get data from it, right? :)

That's how the idea about is_indirectly_device_accessible comes to my mind. Since three of us like it we should probably reach customers to get feedback (as Alexey suggested). Whether we should change "device accessible content iterators", I am not sure. Perhaps, we should (if we agree on the new name) but I don't have a very strong preference

[danhoeflinger]

I adjusted the name here in the spec.

[rarutyun]

I was also thinking that is_device_accessible_iterator partially reflects the purpose. On the other hand we pass iterators to eventually get data from it, right? :)

That's how the idea about is_indirectly_device_accessible comes to my mind. Since three of us like it we should probably reach customers to get feedback (as Alexey suggested). Whether we should change "device accessible content iterators", I am not sure. Perhaps, we should (if we agree on the new name) but I don't have a very strong preference

[rarutyun]

I was fine with keeping "always for counting_iterator and discard_iterator (I think I wrote something like "iterator below". But how it's right now is also good. Please keep it as is.

I have another suggestion somewhere you use "<iterator_name> are" and somewhere "<iterator_name> is". I propose to align to "is" everywhere

Suggested change

	``counting_iterator`` are SYCL device-copyable, and are "device accessible content iterators".
	``counting_iterator`` is SYCL device-copyable, and are "device accessible content iterators".

[danhoeflinger]

adjusted all of these "are" -> "is" [a]

[rarutyun]

Suggested change

	``discard_iterator`` are SYCL device-copyable, and are "device accessible content iterators".
	``discard_iterator`` is SYCL device-copyable, and are "device accessible content iterators".

[danhoeflinger]

adjusted all of these "are" -> "is" [a]

[rarutyun]

Suggested change

	``permutation_iterator`` are SYCL device-copyable if both the ``SourceIterator`` and the ``IndexMap``
	``permutation_iterator`` is SYCL device-copyable if both the ``SourceIterator`` and the ``IndexMap``

[danhoeflinger]

adjusted all of these "are" -> "is" [a]

[rarutyun]

I would still recommend some improvements

Suggested change

	``template <typename T> oneapi::dpl::is_device_accessible_content_iterator<T>`` evaluates to a type with the
	characteristics of ``std::true_type`` if ``T`` refers to "device accessible content", otherwise it evaluates to a type
	with the characteristics of ``std::false_type``.
	``is_device_accessible_content_iterator`` has characteristics of ``std::true_type`` if ``T`` refers to "device accessible content", otherwise it has characteristics of ``std::false_type``.

If you want to keep template <typename T> at the beginning, that's probably fine (though, looks redundant) but please at least remove <T> after class name. Also, "evaluates to a type" sounds a little bit clunky to me.

[danhoeflinger]

fixed.

[rarutyun]

Suggested change

	/* unspecified user definition of a iterator which refers to "device accessible content" */
	/* unspecified user definition of a iterator which refers to "device accessible content" */

[danhoeflinger]

fixed.