[SYCL] Subbuffer CopyBack Data - (REVIEW) #2085

cperkinsintel · 2020-07-10T04:23:34Z

optimization so that when (only) a subbuffer is used, the entire buffer is not copied back.

Signed-off-by: Chris Perkins [email protected]

optimization so that when (only) a subbuffer is used, the entire buffer is not copied back. Signed-off-by: Chris Perkins <[email protected]>

mfbalin · 2020-07-10T05:35:06Z

sycl/test/basic_tests/buffer/subbuffer_data.cpp

+    q.wait();
+
+    // change readBuffer behind its back.
+    clear_arr(baseData);


In the SYCL 2020 provisional specs, it says on page 123, second paragraph that "2. A buffer can be constructed with associated host memory and a default buffer allocator. The buffer will use
this host memory for its full lifetime, but the contents of this host memory are unspecified for the lifetime
of the buffer. If the host memory is modified by the host, or mapped to another buffer or image during the
lifetime of this buffer, then the results are undefined. The initial contents of the buffer will be the contents
of the host memory at the time of construction."

Does this mean that this test has undefined behaviour by SYCL specs because it doesn't use an accessor to access and modify host data that is still in use by a buffer?

I am also wondering whether it is still an undefined behavior to read the host data when its buffer is only accessed via read-accessors throughout its lifetime. The specs say that it is undefined behavior when we try to modify it but it doesn't state whether my use case is allowed or not.

It's UB to access host data passed to the buffer c'tor until the buffer is destroyed.

@cperkinsintel Please, rework the test to avoid UB.

sycl/source/detail/buffer_impl.cpp

KarachunIvan · 2020-07-11T20:18:13Z

sycl/source/detail/buffer_impl.cpp

+  if (Mode == access::mode::read || Mode == access::mode::discard_write ||
+      Mode == access::mode::discard_read_write)


Why discard_write mode isn't a write mode? According to SYCL spec:
access::mode::discard_write | Write-only access. Previous contents discarded

This isn't an exported function. The question isAWriteMode is answering here is "Is this mode one that would have written and that we now need to worry about?". And even though discard_writes do write, we don't need to worry about them - they don't need to propagated.

I think the function should be renamed then because current name is very confusing.

sycl/include/CL/sycl/detail/buffer_usage.hpp

sycl/test/basic_tests/buffer/subbuffer_data.cpp

Signed-off-by: Chris Perkins <[email protected]>

romanovvlad

First round of the comments.

romanovvlad · 2020-07-20T09:34:31Z

sycl/include/CL/sycl/buffer.hpp

@@ -60,81 +60,91 @@ class buffer {
  buffer(const range<dimensions> &bufferRange,
         const property_list &propList = {})
      : Range(bufferRange) {
+    size_t SizeInBytes = get_count() * sizeof(T);


Suggested change

size_t SizeInBytes = get_count() * sizeof(T);

const size_t SizeInBytes = get_count() * sizeof(T);

romanovvlad · 2020-07-20T09:35:24Z

sycl/include/CL/sycl/buffer.hpp

        make_unique_ptr<detail::SYCLMemObjAllocatorHolder<AllocatorT>>());
+    impl->recordBufferUsage(((void *)this), SizeInBytes, 0, false);


Please, add a comment for what literal values stand for, like /*ImprovePerformance2x*/false

romanovvlad · 2020-07-20T09:38:05Z

sycl/include/CL/sycl/buffer.hpp

+    IsSubBuffer = rhs.IsSubBuffer;
+    OffsetInBytes = rhs.OffsetInBytes;


Please, move to initializer list.

romanovvlad · 2020-07-20T09:57:56Z

sycl/include/CL/sycl/detail/buffer_usage.hpp

+};
+
+// need to track information about a sub/buffer,
+// even after its destruction, we may need to know about it.


Why would we want to know about it after sub/buffer destruction?

The sub-buffers handle the copy-back of their own data. They will have been destroyed before the buffer_impl is destroyed. And when it is destroyed it needs to know if it has to initiate a copy back or not. So before it can answer that question it needs to know if there were any sub-buffers and if they actually performed any copy operations (ie had any write accessors declared).

romanovvlad · 2020-07-20T10:01:43Z

sycl/include/CL/sycl/detail/stream_impl.hpp

@@ -108,6 +108,7 @@ class __SYCL_EXPORT stream_impl {
  GlobalOffsetAccessorT accessGlobalOffset(handler &CGH) {
    auto OffsetSubBuf = buffer<char, 1>(Buf, id<1>(0), range<1>(OffsetSize));
    auto ReinterpretedBuf = OffsetSubBuf.reinterpret<unsigned, 1>(range<1>(1));
+    ReinterpretedBuf.set_write_back(false); // Buf handles write back.


Why do you need this change? The spec says:
The reinterpreted SYCL buffer that is constructed must behave as though it were a copy of the SYCL buffer...

Right. And it's a copy of a sub-buffer which, with this new addition, would initiate a copy back on its destruction. The stream implementation is using two accessors, one on the base buffer, and one on the sub-buffer/reinterp. And the reinterp is going out of scope at the end of this function. Don't need it's destructor to initiate a copy back - that's already handled by the base in this case.
I've been thinking about changing how this new sub-buffer dtor code works with reinterprted buffers, such that if a sub-buffer is reinterpreted, then we just go back to the existing system where everything is handled by the base buffer.

romanovvlad · 2020-07-20T10:29:57Z

sycl/test/basic_tests/buffer/subbuffer.cpp

-  for (int i = offset; i < offset + subbuf_size; ++i)
-    assert(vec[i] == (i < offset + offset_inside_subbuf ? i * 10 : i * -10) &&
-           "Invalid result in 1d sub buffer");
+  for (int i = 0; i < size; ++i) {


Does the test fail without the patch?

romanovvlad · 2020-07-20T10:43:56Z

sycl/test/basic_tests/buffer/subbuffer_data.cpp

+
+void ensureNoUnecessaryCopyBack(queue &q) {
+
+  std::cout << "start ensureNoUnecessaryCopyBack" << std::endl;


Please, include iostream if this is used.

romanovvlad · 2020-07-20T10:45:22Z

sycl/test/basic_tests/buffer/subbuffer_data.cpp

+  std::cout << "start ensureNoUnecessaryCopyBack" << std::endl;
+
+  //allocate  memory
+  int *baseData = (int *)(malloc(total * sizeof(int)));


Please use std::vector or std::array.

romanovvlad · 2020-07-20T10:46:04Z

sycl/test/basic_tests/buffer/subbuffer_data.cpp

+  //  But we don't care about the write buffer. We only care that the read buffer was NOT copied-back.
+  { // closure
+    //setup and clear memory
+    setup_arr(baseData);  // [0, 1, 2,  ..., total]


Please, use std::iota instead.

romanovvlad · 2020-07-20T10:46:52Z

sycl/test/basic_tests/buffer/subbuffer_data.cpp

+    q.wait();
+
+    // change readBuffer behind its back.
+    clear_arr(baseData);


@cperkinsintel Please, rework the test to avoid UB.

romanovvlad · 2020-07-21T19:17:35Z

@sergey-semenov Could you review?

cperkinsintel · 2020-07-22T17:20:35Z

This version is the "shortened" one, in that it only deals with copy-backs (not limiting sub-buffer memcpy, for example). So I think it might be a bit overengineered for what it is. I'm going to make a pass at simplifying it.

support for individual subbuffers to copyback data on their destruction

53311b0

optimization so that when (only) a subbuffer is used, the entire buffer is not copied back. Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel requested a review from romanovvlad July 10, 2020 04:23

cperkinsintel requested review from againull and a team as code owners July 10, 2020 04:23

mfbalin reviewed Jul 10, 2020

View reviewed changes

KarachunIvan reviewed Jul 11, 2020

View reviewed changes

romanovvlad marked this pull request as draft July 13, 2020 13:39

add copy constructor for buffer. made suggested changes from review

b45255f

Signed-off-by: Chris Perkins <[email protected]>

romanovvlad requested changes Jul 20, 2020

View reviewed changes

sergey-semenov self-requested a review July 22, 2020 10:55

cperkinsintel mentioned this pull request Jul 23, 2020

[DO NOT MERGE] range/id term ordering issue. #2169

Closed

github-actions bot added the Stale label Feb 18, 2022

github-actions bot closed this Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Subbuffer CopyBack Data - (REVIEW) #2085

[SYCL] Subbuffer CopyBack Data - (REVIEW) #2085

cperkinsintel commented Jul 10, 2020

mfbalin Jul 10, 2020 •

edited

Loading

romanovvlad Jul 10, 2020

romanovvlad Jul 20, 2020

KarachunIvan Jul 11, 2020

cperkinsintel Jul 13, 2020

romanovvlad Jul 20, 2020

romanovvlad left a comment

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

cperkinsintel Jul 20, 2020 •

edited

Loading

romanovvlad Jul 20, 2020

cperkinsintel Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad Jul 20, 2020

romanovvlad commented Jul 21, 2020

cperkinsintel commented Jul 22, 2020

		if (Mode == access::mode::read \|\| Mode == access::mode::discard_write \|\|
		Mode == access::mode::discard_read_write)

	size_t SizeInBytes = get_count() * sizeof(T);
	const size_t SizeInBytes = get_count() * sizeof(T);

		make_unique_ptr<detail::SYCLMemObjAllocatorHolder<AllocatorT>>());
		impl->recordBufferUsage(((void *)this), SizeInBytes, 0, false);

		IsSubBuffer = rhs.IsSubBuffer;
		OffsetInBytes = rhs.OffsetInBytes;


		void ensureNoUnecessaryCopyBack(queue &q) {

		std::cout << "start ensureNoUnecessaryCopyBack" << std::endl;

[SYCL] Subbuffer CopyBack Data - (REVIEW) #2085

[SYCL] Subbuffer CopyBack Data - (REVIEW) #2085

Conversation

cperkinsintel commented Jul 10, 2020

mfbalin Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanovvlad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cperkinsintel Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanovvlad commented Jul 21, 2020

cperkinsintel commented Jul 22, 2020

mfbalin Jul 10, 2020 •

edited

Loading

cperkinsintel Jul 20, 2020 •

edited

Loading