Skip to content

Clarified the condition of buffer write back. #76

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

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

bso-intel
Copy link
Contributor

The current spec is ambiguous about when the data is written back to the original vector/array.
In section 4.7.2.3, the rule #4 says "A buffer can be constructed from a pair of iterator values. In this case, ..... The destructor will not copy back any data and does not need to block"

This patch clarifies the condition when data is written back for the buffer constructor with a pair of iterators.

Signed-off-by: Byoungro So byoungro.so@intel.com

Signed-off-by: Byoungro So <byoungro.so@intel.com>
@CLAassistant
Copy link

CLAassistant commented Mar 6, 2020

CLA assistant check
All committers have signed the CLA.

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 for the clarification.

@bader bader requested a review from AerialMantis March 7, 2020 10:15
@bso-intel
Copy link
Contributor Author

@AerialMantis Could you review this PR? Thanks.

@bso-intel
Copy link
Contributor Author

@AerialMantis Please review this PR. Thanks.

@@ -1237,7 +1237,7 @@ \subsubsection{Buffer Interface}
ranging from \codeinline{first} up to one before \codeinline{last}.
The data is copied to an intermediate memory position by the runtime.
Data is written back to the same iterator set if the iterator is not
a const iterator.
a const iterator and the member function \codeinline{set_final_data()} is called with a valid non-null pointer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This suggests that the data is copied back to the original iterator set if a new destination is set via set_final_data, should this be is not called?

Copy link
Contributor Author

@bso-intel bso-intel Mar 18, 2020

Choose a reason for hiding this comment

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

@AerialMantis
By default, the buffer created from a pair of iterators will not write back to the host memory.
Only when the user calls set_finaL_data(), the buffer is written back to host.
Here is the full sentence after my patch.

Data is written back to the same iterator set if the iterator is not a const iterator and the member function set_final_data() is called with a valid non-null pointer.

Copy link
Member

Choose a reason for hiding this comment

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

It depends on the level of detail in the wording document...
Technically

set_final_data(valid);
set_final_data(nullptr);

cannot trigger the copy back even if set_final_data() is called with a valid non-null pointer... :-)

So perhaps it has to be clarified in term of whether the final data it set to something or not instead of detailing calls to set_final_data()`...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keryell
Yes, there could be two calls to set_final_data(), first call with a valid pointer and then later change it to nullptr.
In fact, most of buffer constructors in Table 4.31 already mentioned the condition of write-back as follows:

Unless the member function set_final_data() is called with a valid non-null pointer there will be no write back on destruction.

However, the spec of set_final_data() in Table 4.32 is clear enough to clarify your case.

If Destination is std::nullptr_t, then the copy back will not happen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By default, the buffer created from a pair of iterators will not write back to the host memory.

Thanks for the clarification, though this wording doesn't seem quite right to me. It also suggests that the data is copied back to the original iterator the buffer is constructed with only if you specify a new final data iterator. So I would interpret this as:

{
  buffer b(begin(dataA), end(dataA);
  b.set_final_data(begin(dataB));
  /* do something with b */
} // copies back to dataA on destruction

The original wording also suggests that data will be copied back if the iterator set is non-const. So if we want the iterators to be treated as input iterators and not copy back by default, then we should also clarify this such that it's never copied back, unless set_final_data is called with a valid destination.

I agree with Ronan, we should have the wording reflect the state of the buffer rather than a specific call to set_final_data.

I would alter the wording to something like:

Data is not written back to the iterator set provided. However, if the buffer has a valid non-const iterator specified via the member function set_final_data() data will be copied back to that iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AerialMantis
Thanks. I will try to use your wording for this buffer constructor and other buffer constructors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AerialMantis @keryell
I fixed wording as you requested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good thanks 👍

…fer.

Signed-off-by: Byoungro So <byoungro.so@intel.com>
@keryell keryell merged commit 1415ee5 into KhronosGroup:SYCL-1.2.1/master Mar 24, 2020
@keryell
Copy link
Member

keryell commented Mar 24, 2020

That looks even better. Thanks!

bader pushed a commit to intel/llvm that referenced this pull request Mar 26, 2020
Fixed the buffer constructor called with a pair of iterators.
The current implementation has a problem due to ambiguous spec.
The buffer should never write back data unless there is a call to set_final_data(), but the current implementation does it.
I corrected the spec in KhronosGroup/SYCL-Docs#76.
So, now we can change the buffer implementation according to the clarified spec.

The test case buffer.cpp also needed change because of this change.
The user should not expect the automatic write-back of data upon destruction of buffer.

Signed-off-by: Byoungro So <byoungro.so@intel.com>
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
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.

4 participants