-
Notifications
You must be signed in to change notification settings - Fork 808
[SYCL] adds marray class as defined by SYCL 2020 provisional #2928
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
Conversation
Signed-off-by: iburylov <[email protected]>
Signed-off-by: iburylov <[email protected]>
@iburyl , please point to the spec bits that this implements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sycl.hpp
has changed its permissions. Is that necessary?
Other comments are minor nits.
This is chapter |
…reverted back permission for sycl.hpp Signed-off-by: iburylov <[email protected]>
I am sure the failed test on memory consumption is not related to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Should there be any tests on added class behaviour?-
Sorry, I've missed a file there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistic comments.
Could you please use the same style of naming types and vars? Things defined by the spec, probably, can be ignored.
Signed-off-by: iburylov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not merge that fast around vacation time, when the review bandwidth is probably lower...
typename = typename std::enable_if<sizeof...(ArgTN) == NumElements>::type> | ||
marray(const ArgTN &... Args) : MData{Args...} {} | ||
|
||
marray(const marray<Type, NumElements> &Rhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to define explicitly these 2 copy and move constructors instead of relying on the default one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly because they are explicitly present in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting side effect of the specification...
In the specification we say that they do exist, not that you have to implement them explicitly if the compiler do this for you in the implementation. :-)
Probably you can remove them for sycl::vec
and so on.
You deserve some laziness by using some = default;
when required. :-)
In the same way you do not define the destructor even it is described in the common property of SYCL objects.
|
||
const_reference operator[](std::size_t index) const { return MData[index]; } | ||
|
||
marray &operator=(const marray<Type, NumElements> &Rhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not relying on the synthesized one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is explicitly present in the spec.
return Ret; | ||
} | ||
|
||
friend marray operator+(marray &Lhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are up to date to our latest bug fixes! Good. :-)
But there is a missing const
// && dataT != cl_half | ||
template <typename T = DataT> | ||
friend typename std::enable_if<std::is_integral<T>::value, marray>::type | ||
operator~(marray &Lhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that there are a lot of const
lacking in the neighborhood
operator~(const marray &Lhs) {
sycl::marray<int, 1> marray_from_one_elem(1); | ||
|
||
// Check broadcasting operator= | ||
sycl::marray<float, 4> b_marray(1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably in the following we need more const
or rvalue to test that it can compile in that case...
Implements marray class type, its operators and aliases as defined by SYCL 2020 provisional
Signed-off-by: iburylov [email protected]