-
Notifications
You must be signed in to change notification settings - Fork 113
[oneDPL] Indirectly Device Accessible Iterator Customization Point and Public Trait #620
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.
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
Changes from 23 commits
d90682b
24b879d
ff940e8
0c0804b
8cff3e4
a36a006
cb102cb
bc32ce2
e7ffb99
913d555
29e209f
180a500
a852f0f
41b9828
96b1b45
dd71fdf
21b8d0f
7c76593
250aed2
696d925
b250e77
8928a06
1fff43d
59c147a
0fe3101
1d44b35
a918a45
25edb0b
b4aeaf0
4967f43
583fd52
b8a6018
311f2cb
8676878
ef67c3d
0f64be0
780f501
a57c7d7
6114c2c
536f2fa
632ac23
b5f1d8c
3fa2a02
1c71426
3b94837
699562b
1369be7
7e02e6a
7d07c92
8e32c96
5cee0b6
3970f44
d00d9cd
98a0cf4
9499cdd
d239878
4080952
00c6c7e
524f8f0
7616251
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,9 +3,31 @@ | |||||||||
.. | ||||||||||
.. SPDX-License-Identifier: CC-BY-4.0 | ||||||||||
|
||||||||||
.. _iterators: | ||||||||||
|
||||||||||
Iterators | ||||||||||
--------- | ||||||||||
|
||||||||||
Requirements For Iterator Use With Device Policies | ||||||||||
akukanov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
++++++++++++++++++++++++++++++++++++++++++++++++++ | ||||||||||
Iterator are, by default, not assumed to refer to content which is accessible on the device which requires the content | ||||||||||
to be copied to the device prior to being used inside a `SYCL`_ kernel. We define iterators to be "device accessible | ||||||||||
content iterators" when they can inherently be dereferenced on the device in a SYCL kernel. When using oneDPL algorithms | ||||||||||
with a ``device_policy``, "device accessible content iterators" avoid unnecessary data movement when provided as input | ||||||||||
or output arguments. | ||||||||||
|
||||||||||
Examples of "device accessible content iterators" include SYCL USM shared or device memory, or iterator types like | ||||||||||
``counting_iterator`` or ``discard_iterator`` that do not require any data to be copied to the device to be used in a | ||||||||||
SYCL kernel. An example of an iterator which does not refer to "device accessible content" is a ``std::vector`` | ||||||||||
iterator, which requires the data to be copied to the device in some way prior to usage in a SYCL kernel within | ||||||||||
algorithms used with a ``device_policy``. | ||||||||||
|
||||||||||
When used as input or output with algorithms using a ``device_policy`` "device accessible content iterators" must also | ||||||||||
be SYCL device-copyable, as defined by the SYCL Specification. | ||||||||||
|
||||||||||
oneDPL Iterators | ||||||||||
++++++++++++++++ | ||||||||||
|
||||||||||
The oneDPL iterators are defined in the ``<oneapi/dpl/iterator>`` header, | ||||||||||
in ``namespace oneapi::dpl``. | ||||||||||
|
||||||||||
|
@@ -64,6 +86,8 @@ counter; dereference operations cannot be used to modify the counter. The arithm | |||||||||
operators of ``counting_iterator`` behave as if applied to the values of Integral type | ||||||||||
representing the counters of the iterator instances passed to the operators. | ||||||||||
|
||||||||||
``counting_iterator`` are always SYCL device-copyable, and are always "device accessible content iterators". | ||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
class discard_iterator | ||||||||||
|
@@ -104,6 +128,8 @@ lvalue that may be assigned an arbitrary value. The assignment has no effect on | |||||||||
of ``discard_iterator`` behave as if applied to integer counter values maintained by the | ||||||||||
iterator instances to determine their position relative to each other. | ||||||||||
|
||||||||||
``discard_iterator`` are always SYCL device-copyable, and are always "device accessible content iterators". | ||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
template <typename SourceIterator, typename IndexMap> | ||||||||||
|
@@ -174,6 +200,10 @@ to index into the index map. The corresponding value in the map is then used | |||||||||
to index into the value set defined by the source iterator. The resulting lvalue is returned | ||||||||||
as the result of the operator. | ||||||||||
|
||||||||||
``permutation_iterator`` are SYCL device-copyable if both the ``SourceIterator`` and the ``IndexMap`` | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adjusted all of these "are" -> "is" [a] |
||||||||||
are SYCL device-copyable, and are always "device accessible content iterators" if both the ``SourceIterator`` | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
"Always" word seems redundant to me. The same comment is applicable for the iterators below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed all instances of this. |
||||||||||
and the ``IndexMap`` are "device accessible content iterators". | ||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
template <typename SourceIterator, typename IndexMap> | ||||||||||
|
@@ -234,6 +264,10 @@ arithmetic and comparison operators of ``transform_iterator`` behave as if appli | |||||||||
source iterator itself. The template type ``Iterator`` must satisfy | ||||||||||
``AdaptingIteratorSource``. | ||||||||||
|
||||||||||
``transform_iterator`` is SYCL device-copyable if the source iterator is SYCL device-copyable, and | ||||||||||
is always "device accessible content iterator" if the source iterator is a "device accessible | ||||||||||
content iterator". | ||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
template <typename UnaryFunc, typename Iterator> | ||||||||||
|
@@ -293,6 +327,10 @@ source iterators over which the ``zip_iterator`` is defined. The arithmetic oper | |||||||||
operation were applied to each of these iterators. The types ``T`` within the template pack | ||||||||||
``Iterators...`` must satisfy ``AdaptingIteratorSource``. | ||||||||||
|
||||||||||
``zip_iterator`` is SYCL device-copyable if all the source iterators are SYCL device-copyable, and | ||||||||||
is always "device accessible content iterator" if all the source iterators are "device accessible | ||||||||||
content iterators". | ||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
template <typename... Iterators> | ||||||||||
|
@@ -301,3 +339,105 @@ operation were applied to each of these iterators. The types ``T`` within the te | |||||||||
|
||||||||||
``make_zip_iterator`` constructs and returns an instance of ``zip_iterator`` | ||||||||||
using the set of source iterators provided. | ||||||||||
|
||||||||||
.. _iterators-device-accessible: | ||||||||||
|
||||||||||
Customization For User Defined Iterators | ||||||||||
akukanov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
++++++++++++++++++++++++++++++++++++++++ | ||||||||||
|
||||||||||
oneDPL provides a mechanism to indicate whether custom iterators are "device accessible content iterators" by defining | ||||||||||
an Argument-Dependent Lookup (ADL) customization point, ``is_onedpl_device_accessible_content_iterator``. oneDPL also | ||||||||||
danhoeflinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
defines a public trait, ``is_device_accessible_content_iterator[_v]`` to indicate whether an iterator is a "device | ||||||||||
accessible content iterator". | ||||||||||
|
||||||||||
oneDPL queries this information at compile time to determine how to handle iterator types when they are passed to | ||||||||||
algorithms with a ``device_policy`` to avoid unnecessary data movement. | ||||||||||
|
||||||||||
ADL Customization Point: ``is_onedpl_device_accessible_content_iterator`` | ||||||||||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||||||
|
||||||||||
A free function ``is_onedpl_device_accessible_content_iterator(IteratorT)`` may be defined, which accepts an argument | ||||||||||
of type ``IteratorT`` and returns a type with the characteristics of ``std::true_type`` if ``IteratorT`` refers to | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not true for a generic case. As I mentioned, I believe I believe that this sentence about characteristics has a perfect fit to where you describe Here we need something else. Don't have immediate suggestions, though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it is used with
And that's exactly our goal I believe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose I don't know exactly what we mean by "characteristics of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, it's even more strict - the standard requires public inheritance, see https://eel.is/c++draft/meta.rqmts#:Cpp17UnaryTypeTrait:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 However, I think a valid implementation could easily accommodate arbitrary usage of |
||||||||||
"device accessible content", or alternatively returns a type with the characteristics of ``std::false_type`` | ||||||||||
danhoeflinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
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``. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
You may leave this part, if you think it's useful: "... , which includes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken, I removed the second part. It doesn't really make sense after the edit. Its more something for the documentation than the spec. |
||||||||||
|
||||||||||
The function ``is_onedpl_device_accessible_content_iterator`` may be used by oneDPL to determine if the iterator refers | ||||||||||
to "device accessible content" by interrogating its return type at compile-time only. It shall not be called by oneDPL | ||||||||||
outside a ``decltype`` context to determine the return type. Overloads may be provided as forward declarations only, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it's important. I would probably remove it.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it justifies the technical reason that "Overloads may be provided as forward declarations only,...", but perhaps its just enough to say that. I've removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but the thing about With the current design I am glad that you decided to remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I understand this most recent comment. As it stands, users are able to provide either declarations or full definitions. As we discussed offline previously, return type must be knowable at compile-time regardless of what is marked as I'm not understanding why knowing the Can you help me understand with an example of something allowed for a user in with this |
||||||||||
without a body defined. ADL lookup is used to determine which function overload to use according to the rules in the | ||||||||||
danhoeflinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
`C++ Standard`_. Therefore, derived iterator types without an overload for their exact type will match their most | ||||||||||
specific base iterator type if such an overload exists. | ||||||||||
|
||||||||||
The default implementation of ``is_onedpl_device_accessible_content_iterator`` marks the following iterators as to | ||||||||||
"device accessible content iterators": | ||||||||||
* Pointers (to handle USM pointers) | ||||||||||
* Iterators to USM shared allocated ``std::vector``-s when the allocator type is knowable from the iterator type | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it ever a case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice, with a good amount of certainty with some compilers, yes. However, its a good question if its worth mentioning in the specification, since it is not always knowable (and not 100% knowable I believe). How it looks in the oneDPL's implementation currently: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think of that as of specification, not as of documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice, oneDPL's implementation is relying upon standard library implementation details which are not part of the C++ standard library specification of In practice, it would be quite unlikely that a standard library implementation could / would have a From a specification perspective, it doesn't belong, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the line for now, but I'm open to discussion of course. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, if we speak about As you can see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we re-introduce something like this, we can describe it with |
||||||||||
* ``std::reverse_iterator<IteratorT>`` when ``IteratorT`` refers to "device accessible content" | ||||||||||
|
||||||||||
|
||||||||||
|
||||||||||
Public Trait: ``oneapi::dpl::is_device_accessible_content_iterator[_v]`` | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming suggestions to consider
For the last one it might be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also thinking that That's how the idea about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I adjusted the name here in the spec. |
||||||||||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||||||
|
||||||||||
The public trait ``oneapi::dpl::is_device_accessible_content_iterator[_v]`` can be used to query whether an iterator | ||||||||||
is a "device accessible content iterator". The trait is defined in ``<oneapi/dpl/iterator>``. | ||||||||||
|
||||||||||
``oneapi::dpl::is_device_accessible_content_iterator<T>`` evaluates to a type with the characteristics of | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjusted to be close to your proposal. |
||||||||||
``std::true_type`` if ``T`` refers to "device accessible content", otherwise it evaluates to a type with the | ||||||||||
characteristics of ``std::false_type``. | ||||||||||
|
||||||||||
``oneapi::dpl::is_device_accessible_content_iterator<T>`` is a ``constexpr bool`` that evaluates to ``true`` if ``T`` | ||||||||||
danhoeflinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
refers to "device accessible content", otherwise it evaluates to ``false``. | ||||||||||
|
||||||||||
|
||||||||||
|
||||||||||
|
||||||||||
Example | ||||||||||
^^^^^^^ | ||||||||||
|
||||||||||
The following example shows how to define a ADL overload for a simple user defined iterator. It also shows an example | ||||||||||
for a more complicated case which uses a hidden friend to provide the ADL overload within the definition of the | ||||||||||
iterator. | ||||||||||
danhoeflinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
.. code:: cpp | ||||||||||
|
||||||||||
namespace usr | ||||||||||
{ | ||||||||||
struct accessible_it | ||||||||||
{ | ||||||||||
/* unspecified user definition of a iterator which refers to "device accessible content" */ | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||||||||||
}; | ||||||||||
|
||||||||||
std::true_type | ||||||||||
is_onedpl_device_accessible_content_iterator(accessible_it); | ||||||||||
|
||||||||||
struct inaccessible_it | ||||||||||
{ | ||||||||||
/* unspecified user definition of iterator which doesn't refer to "device accessible content" */ | ||||||||||
}; | ||||||||||
|
||||||||||
// This could be omitted. It would rely upon the default implementation with the same result | ||||||||||
std::false_type | ||||||||||
is_onedpl_device_accessible_content_iterator(inaccessible_it); | ||||||||||
} | ||||||||||
|
||||||||||
static_assert(oneapi::dpl::is_device_accessible_content_iterator<usr::accessible_it> == true); | ||||||||||
static_assert(oneapi::dpl::is_device_accessible_content_iterator<usr::inaccessible_it> == false); | ||||||||||
|
||||||||||
// Example with base iterators and ADL overload as a hidden friend | ||||||||||
template <typename It1, typename It2> | ||||||||||
struct it_pair | ||||||||||
{ | ||||||||||
It1 first; | ||||||||||
It2 second; | ||||||||||
friend auto is_onedpl_device_accessible_content_iterator(it_pair) -> | ||||||||||
std::conjunction<oneapi::dpl::is_device_accessible_content_iterator<It1>, | ||||||||||
oneapi::dpl::is_device_accessible_content_iterator<It2>>; | ||||||||||
}; | ||||||||||
|
||||||||||
static_assert(oneapi::dpl::is_device_accessible_content_iterator<it_pair<usr::accessible_it, usr::accessible_it>> == true); | ||||||||||
static_assert(oneapi::dpl::is_device_accessible_content_iterator<it_pair<usr::accessible_it, usr::inaccessible_it>> == false); | ||||||||||
|
||||||||||
.. _`C++ Standard`: https://isocpp.org/std/the-standard | ||||||||||
.. _`SYCL`: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html |
Uh oh!
There was an error while loading. Please reload this page.