Clarify buffer and accessor lifetime

[gmlueck]

Clarify that the application must not use an accessor after its
associated buffer / image is destroyed, Also clarify the point at
which requisites are added for a command. This happens after the
command group function object returns, not during the call to
parallel_for (etc.)

Closes internal issue 420

[fraggamuffin]

LGTM

[keryell]

Diving more into it, I have the feeling it requires more discussion.
Selecting Request changes to avoid merging for now.

[keryell]

I do not understand how it is an issue in a conforming implementation.
If you you have a live accessor, the buffer is still kept alive though for example an internal shared_ptr.

[keryell]

The case of the host accessor looks simpler, very synchronous without any lazy lambda evaluation and should keep a buffer internal state alive.

[keryell]

The problem I can see in my implementation is more like a deadlock.
But thinking more about it, I am unsure that not dealing with dependencies at accessor construction time instead of postponing after submit lambda execution can be a generic enough C++ implementation.

[keryell]

It looks more like implementation detail & trade-off, so I can give up my previous comment.

[keryell]

I have the feeling that the buffer destructor should block if there are accessors using it or at least being internally kept alive if there is no side effect on buffer use or buffer destruction.

[keryell]

I can give up on this.

[keryell]

Should we have more content about image lifetime and synchronization?
I have the feeling we only addressed buffers up to now in the specification.

[keryell]

Your comment simplify the implementation, so I am fine with it.

[keryell]

I have the feeling it depends only on construction and not member functions.

[keryell]

I am still thinking that the host_accessor could keep a buffer alive internally.

[keryell]

The case of the host accessor looks simpler, very synchronous without any lazy lambda evaluation and should keep a buffer internal state alive.

[keryell]

I can give up on this.

[keryell]

It looks more like implementation detail & trade-off, so I can give up my previous comment.

[keryell]

Your comment simplify the implementation, so I am fine with it.

[gmlueck]

I am still thinking that the host_accessor could keep a buffer alive internally.

What about a case like this:

host_accessor<int, 1> acc;
{
  int array[] = {1, 2, 3, 4};
  buffer b(array, range{4});
  acc = b.get_host_access(b);
}
bool test = (acc[0] == 1);  // Well defined?

Note that both the buffer and the underlying array go out of scope while there is still a live accessor. Are you suggesting that the host accessor should make a copy of the array data, so that the last line still works even though the underlying buffer and array no longer exist?

To me, it seems better to adopt one of these two clarifications:

• Say that this is UB (the wording I have currently in this PR), or
• Say that the buffer destructor blocks while there are any outstanding accessors to it, which I think you proposed in another comment. Thus, the code above would lead to a deadlock.

[gmlueck]

I converted this to a draft because there is at least one thing that needs to be fixed before it can be merged. Recent discussion in the internal issue 420 revealed that the current wording of this PR causes the following code snippet to be UB:

event e;
{
  buffer b{range{3}};
  e = q.submit([&](handler &cgh) {
    accessor a{b, cgh};
    cgh.single_task([=] {
      a[0] = 42;
    });
  });
  /* buffer goes out of scope */
}
/* command could run here */
e.wait();

The current PR makes this UB because the command could run even after the buffer is destroyed.

Code like this is currently legal according to the current wording of SYCL 2020, and I think we want to continue to allow code like this. I see two options:

• Use the current wording in this PR, but make an exception for the case when a buffer is constructed without underlying host memory. In such a case, we say that it is legal to use an accessor even after the underlying buffer is destroyed, or

• Say that the buffer destructor blocks while there are outstanding accessors to it. In the code snippet above, the buffer destructor would block until the command completes. Thus, the call to e.wait() is superfluous.

[fraggamuffin]

use cases and examples needed

[keryell]

Book 1 hour in advance to discuss this.

[fraggamuffin]

a good F2F

[keryell]

Thanks for the great presentation today explaining your proposal with all the corner cases about the life of buffer and accessor!
In triSYCL the accessor implementation extends the lifetime of the internal detail::buffer by keeping a reference on it:
https://github.com/triSYCL/triSYCL/blob/master/include/triSYCL/buffer/detail/accessor.hpp#L66
@illuhad I guess you might do something similar?

[keryell]

@gmlueck can you send me your presentation so I can make a new version commenting your examples with a different hypothesis?

[keryell]

I have update my presentation about this on the Khronos-internal https://members.khronos.org/wg/SYCL/document/31310