-
Notifications
You must be signed in to change notification settings - Fork 68
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
Move description of shared_ptr buffer destructor #298
base: main
Are you sure you want to change the base?
Conversation
The buffer destructor has special rules about blocking when the buffer is constructed from a `shared_ptr`. Previously, these rules were described in section 4.7.4.3 "Shared SYCL ownership of the host memory". However, the behavior of the buffer destructor in other cases is described in section 4.7.2.3 "Buffer synchronization rules". This PR moves the description to 4.7.2.3 in order to keep the descriptions of the buffer destructor all in one place. After doing this, I realized that section 4.7.4.3 did not contain any new information that wasn't already described in 4.7.2.3. Therefore, I removed that section. I also tightened the specification a bit for the case when a buffer is constructed from a `shared_ptr`: * If the underlying type of the `shared_ptr` is `const`, then I assume we want the buffer to be read-only, just as in the raw pointer case. * I also assume that the contents of the host memory referenced by the `shared_ptr` is undefined during the lifetime of the buffer, just as in the raw pointer case.
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.
Globally that sounds like a good simplification and clarification.
} | ||
---- | ||
|
||
|
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 we keep these examples?
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 checked again : these were the only 2 code examples showing sycl::buffer
and std::shared_ptr
. Why removing them? Are they wrong now?
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 agree that the examples are good to keep. I was hoping to move all of the normative description of the buffer APIs to section 4.7.2 Buffers, and in particular the normative description of the blocking behavior and copy-back behavior to section 4.7.2.3 Buffer synchronization rules.
How do you feel about the following ... I'll still augment section 4.7.2.3 to contain a complete description of the blocking behavior and copy-back behavior, so all of that information is together in one place. We can still keep section 4.7.4, though, as a set of examples. In this case, I'd probably retitle the section to something like "Buffer examples", and the introduction of that section will read something like:
This section provides some examples showing typical use cases for buffer. These examples are intended to clarify the definition of the buffer interfaces, but the content of this section is nonnormative.
Over time, we can add more examples to this section to clarify other behavior about buffers.
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 restored the examples, and converted section 4.7.4 to an examples section as I describe in the comment above. See commit 15e8eab.
Note that this PR is incomplete at this point because I need answers to the questions in (https://gitlab.khronos.org/sycl/Specification/-/issues/478)
buffer<int, 1> b { ptr, range<2>{ 10, 10 } }; | ||
// update the data | ||
[...] | ||
ptr.reset(); |
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.
This line require a comment, for casual readers not using routinely std::shared_ptr
.
} | ||
---- | ||
|
||
|
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 checked again : these were the only 2 code examples showing sycl::buffer
and std::shared_ptr
. Why removing them? Are they wrong now?
* Convert section 4.7.4 "Sharing host memory with the SYCL data management classes" into a non-normative set of examples and retitle the section "Example buffer usage". This restores the examples removed in the previous commit, adds some new examples, and moves the existing example about sub-buffers to this section. * The existing example for sub-buffers was updated to use SYCL 2020 features that reduce its verbosity. * Add more clarifications to section 4.7.2.3 "Buffer synchronization rules" and reorder some of the information to make it flow better. The descriptions of `set_final_data` and `set_write_back` are incomplete, pending answers to questions in internal issue 478.
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.
Great clarification!
Dedicate some time like 1 hour to solve this after some preparing time. |
A good F2F |
In general, the buffer destructor blocks under two circumstances: if the buffer | ||
assume exclusive access to some host memory passed to its constructor or if | ||
some data needs to be copied back to the host upon completion of commands | ||
associated with the buffer. |
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.
In general, the buffer destructor blocks under two circumstances: if the buffer | |
assume exclusive access to some host memory passed to its constructor or if | |
some data needs to be copied back to the host upon completion of commands | |
associated with the buffer. | |
In general, the buffer destructor blocks under two circumstances: if the buffer | |
assumes exclusive access to some host memory passed to its constructor or if | |
some data needs to be copied back to the host upon completion of commands | |
associated with the buffer. |
The following paragraphs describe these rules more precisely for each overload | ||
of the buffer constructor: |
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 think we're missing the sub-buffer overload here. I see no explicit mention of sub-buffer synchronization rules neither. In the original text, sub-buffers are mentioned at the end of the section, but no synchronization rules are specified. I assume sub-buffers destruction does not block, only the buffer one would, right?
The buffer destructor has special rules about blocking when the buffer is constructed from a
shared_ptr
. Previously, these rules were described in section 4.7.4.3 "Shared SYCL ownership of the host memory". However, the behavior of the buffer destructor in other cases is described in section 4.7.2.3 "Buffer synchronization rules". This PR moves the description to 4.7.2.3 in order to keep the descriptions of the buffer destructor all in one place.After doing this, I realized that section 4.7.4.3 did not contain any new information that wasn't already described in 4.7.2.3. Therefore, I removed that section.
I also tightened the specification a bit for the case when a buffer is constructed from a
shared_ptr
:If the underlying type of the
shared_ptr
isconst
, then I assume we want the buffer to be read-only, just as in the raw pointer case.I also assume that the contents of the host memory referenced by the
shared_ptr
is undefined during the lifetime of the buffer, just as in the raw pointer case.