[SYCL] Check if the underlying buffer type is device copyable #4914
Conversation
The implementation should diagnose an error if the underlying type "T" of a buffer is not device copyable. The spec says that this is a requirement for all buffers. It does not make any exception for buffers that are used only on the host: "The underlying data type of a buffer T must be device copyable as defined in Section 3.13.1." (https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:buffers)
See commit 43c6d7a4e1f78, the sycl::vec class is not trivially copyable on the host side, so it is not considered as device copyable. A partial specialization for the sycl::is_device_copyable for sycl::vec is implemented to mark the class as device copyable on the host side.
ghost
requested review from
|
Since the |
ghost
mentioned this pull request
sycl/include/CL/sycl/types.hpp
Outdated
| #ifndef __SYCL_DEVICE_ONLY__ | ||
| template <typename T, int N> | ||
| struct is_device_copyable<sycl::vec<T, N>, | ||
| std::enable_if_t<is_device_copyable<T>::value>> |
There was a problem hiding this comment.
I don't think you need this enable_if. According to the spec, the underlying type dataT of vec "must be one of the basic scalar types supported in device code". I think this means that all possible types dataT will be trivially copyable (and thus device copyable).
There was a problem hiding this comment.
+1 on this comment. While this file is too crowded with templates, that removing this one clause will not make any difference at all, it is still a good idea not to produce unnecessary template instantiations.
There was a problem hiding this comment.
@gmlueck @alexbatashev Thanks for the suggestion. I've removed the check for the underlying type T.
sycl/include/CL/sycl/types.hpp
Outdated
| #ifndef __SYCL_DEVICE_ONLY__ | ||
| template <typename T, int N> | ||
| struct is_device_copyable<sycl::vec<T, N>, | ||
| std::enable_if_t<is_device_copyable<T>::value>> |
There was a problem hiding this comment.
+1 on this comment. While this file is too crowded with templates, that removing this one clause will not make any difference at all, it is still a good idea not to produce unnecessary template instantiations.
| #include <sycl/sycl.hpp> | ||
| #include <string> | ||
|
|
||
| using namespace cl::sycl; |
There was a problem hiding this comment.
| using namespace cl::sycl; | |
| using namespace sycl; |
There was a problem hiding this comment.
@alexbatashev Thank you, the namespace is changed to sycl::.
The class 'point' is used to test 'multi_ptr'. A 'sycl::buffer' for instances of the class is created and the class must be a trivially copyable to be trait as device copyable. Otherwise a compilation error will be generated after a new check for device copyability in the 'sycl::buffer' class, see intel/llvm#4914. Signed-off-by: Pavel Samolysov <[email protected]>
According to the spec, the underlying type dataT of vec "must be one of the basic scalar types supported in device code". This means that all possible types dataT will be trivially copyable (and thus device copyable).
ghost
mentioned this pull request
…lvm-test-suite#555) The class 'point' is used to test 'multi_ptr'. A 'sycl::buffer' for instances of the class is created and the class must be a trivially copyable to be trait as device copyable. Otherwise a compilation error will be generated after a new check for device copyability in the 'sycl::buffer' class, see intel#4914. Signed-off-by: Pavel Samolysov <[email protected]>
The implementation should diagnose an error if the underlying type "T"
of a buffer is not device copyable. The spec says that this is
a requirement for all buffers. It does not make any exception for
buffers that are used only on the host:
"The underlying data type of a buffer T must be device copyable as
defined in Section 3.13.1."
(https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:buffers)