Clarify group algorithm constraints #730
Conversation
This commit makes two clarifications: - When an algorithm accepts a pointer, it can be a multi_ptr; and - When an algorithm accepts a value, it can be a vec or marray. A list of types compatible with group algorithms is removed in favor of listing the types explicitly for each algorithm, since some algorithms have different constraints.
This note had been duplicated for group functions and group algorithms. The note about group algorithms was removed in a previous commit.
|
Look good, but should CTS but updated? |
| . _Constraints:_ Available only if all of the following conditions are met: | ||
| - [code]#sycl::is_group_v<std::decay_t<Group>># is true; and | ||
| - [code]#Ptr# is a pointer or [code]#multi_ptr#. | ||
|
|
There was a problem hiding this comment.
Just checking ... because you removed the general statements about the types that are supported by the group algorithms, there is no longer any constraint on T or on the type that Ptr can point to. Is that your intent?
Just pointing this out because the PR comment seems to imply that you intended to constraint the types allowed in group algorithms to the fundamental types, half, vec, and marray.
| sub_group># is true and [code]#T# is a trivially copyable type. | ||
| . _Constraints:_ Available only if all of the following conditions are met: | ||
| - [code]#sycl::is_same_v<std::decay_t<Group>, sub_group># is true; and | ||
| - [code]#T# is a trivially copyable type. |
There was a problem hiding this comment.
Note that the spec does not guarantee that vec or marray are trivially copyable. I believe there is an internal issue which suggests that we should change this. At one point, DPC++ did not implement these as trivially copyable, though I think we may have fixed that by now.
| . _Constraints:_ Available only if all of the following conditions are met: | ||
| - [code]#sycl::is_group_v<std::decay_t<Group>># is true; | ||
| - [code]#Ptr# is a pointer or [code]#multi_ptr# to: a fundamental type, | ||
| [code]#sycl::half#, [code]#marray# or [code]#vec#; and |
There was a problem hiding this comment.
Note that marray is allowed to have elements that are not fundamental types. In fact, the elements are even allowed to be types that are not trivially copyable (or even device copyable). I suspect you do not intend to support marray in these cases?
| - [code]#Ptr# is a pointer or [code]#multi_ptr# to: a fundamental type, | ||
| [code]#sycl::half#, [code]#marray# or [code]#vec#; and | ||
| - [code]#BinaryOperation# is a SYCL function object type. | ||
|
|
||
| + | ||
| -- | ||
| _Mandates:_ [code]#binary_op(*first, *first)# must return a value of type |
There was a problem hiding this comment.
(This comment is for the line below, but GitHub won't let me make a comment there.)
I'm sure this is opening a can of worms, but we do not say anywhere that std::iterator_traits is specialized for multi_ptr. This becomes relevant with this PR because it is clarifying that Ptr can be a multi_ptr.
This PR makes three clarifications:
multi_ptr;vecormarray;sycl::half.A list of types compatible with group algorithms is removed in favor of listing the types explicitly for each algorithm, since some algorithms have different constraints (e.g., some algorithms support all trivially copyable types).
Closes #340 and closes #461.
Two other things to note here...
I deliberately didn't add
__swizzle__, because I don't think that was part of our original intent. We could explore this as an extension, but adding the overloads for__swizzle__is non-trivial; the return type would have to be different (i.e.,vec), and I don't think existing implementations actually support this case.I realize that the group algorithms section is now really long and repetitive, but my goal with this PR is simply to fix the wording. The section would definitely benefit from another formatting and consolidation pass. Defining a term that means "C++ fundamental type,
half,marrayorvecwould also help.