Skip to content
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

Type for copy and fill should be device-copyable #425

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Jun 7, 2023

Clarify that the type used in the handler::copy and handler::fill functions must be "device copyable". This avoids confusion when T is a class type because people might expect the class's copy constructor to be called, when our intent is that the implementation should just do a byte copy.

There is no need to clarify the element type of the accessors in these functions becuse we already state that the element type of a buffer must be device copyable. Therefore, this PR only changes the wording for the non-accessor parameters.

Clarify that the type used in the `handler::copy` and `handler::fill`
functions must be "device copyable".  This avoids confusion when `T`
is a class type because people might expect the class's copy
constructor to be called, when our intent is that the implementation
should just do a byte copy.

There is no need to clarify the element type of the accessors in these
functions becuse we already state that the element type of a buffer
must be device copyable.  Therefore, this PR only changes the wording
for the non-accessor parameters.
Copy link
Contributor

@TApplencourt TApplencourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any prefetch or mem_advise that are templated with T so LGTM!

Copy link
Contributor

@illuhad illuhad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@fraggamuffin
Copy link

LGTM

@keryell keryell merged commit 09f8a2e into KhronosGroup:SYCL-2020/master Jun 8, 2023
@gmlueck gmlueck deleted the gmlueck/copy-fill-device-copyable branch June 8, 2023 17:14
AlexeySachkov pushed a commit to intel/llvm that referenced this pull request Jun 12, 2023
…#8826)

`cgh.fill(...)` was failing when `T` is a `sycl::vec` type, since
`sycl::vec` types have no specialization for
`std::is_trivially_copyable`. Since `device_copyable` is a sycl superset
of trivially copyable we should use this instead in sycl headers.

Related spec change: KhronosGroup/SYCL-Docs#425
0x12CC added a commit to 0x12CC/llvm that referenced this pull request Jun 12, 2023
KhronosGroup/SYCL-Docs#425 clarifies that the source type for the `copy`
must be device copyable. Remove the copy from `const void *` test case
since it fails this assertion.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Jun 13, 2023
KhronosGroup/SYCL-Docs#425 clarifies that the source type for the `copy`
must be device copyable. Remove the copy from `const void *` test case
since it fails this assertion.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
fineg74 pushed a commit to fineg74/llvm that referenced this pull request Jun 15, 2023
KhronosGroup/SYCL-Docs#425 clarifies that the source type for the `copy`
must be device copyable. Remove the copy from `const void *` test case
since it fails this assertion.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
keryell added a commit that referenced this pull request Sep 10, 2024
Clarify that the type used in the handler::copy and handler::fill functions must be "device copyable". This avoids confusion when T is a class type because people might expect the class's copy constructor to be called, when our intent is that the implementation should just do a byte copy.

There is no need to clarify the element type of the accessors in these functions becuse we already state that the element type of a buffer must be device copyable. Therefore, this PR only changes the wording for the non-accessor parameters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants