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

Default constructed accessors #89

Closed
wants to merge 2 commits into from

Conversation

ProGTX
Copy link
Contributor

@ProGTX ProGTX commented Jan 24, 2019

This relates to #18.

It introduces a default constructed placeholder accessor, similar to that patch,
called a null accessor here.
Instead of proposing a require(buf, acc),
the buffer can be "bound" to an accessor by creating a new placeholder accessor
that is already bound to a buffer.
It can then be used as a normal placeholder accessor.
If it's not bound, it can still be passed into a kernel,
but should not be dereferenced inside the kernel.

A new free function make_placeholder_accessor accessor is also proposed.


Not providing either the handler or the memory object
makes the accessor default constructible,
creating a "null" accessor.
Copy link
Contributor

Choose a reason for hiding this comment

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

a "null" accessor -> a "null" or "unbound" placeholder accessor. This represents just the placeholder (i.e. the space that will be occupied by the data type) that needs to be associated with a buffer at command group submission time.

It is allowed to pass a null accessor to a kernel,
as long as it's not dereferenced inside the kernel,
otherwise an out-of-bounds access will occur,
likely leading to a segmentation fault or similar.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite ambiguous. Is better simply to say a device-specific asynchronous error

If the null accessor has been bound to a buffer,
it is no longer a null accessor,
which means it has to be registered with the command group
and can be dereferenced inside the kernel.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also a bit confusing. A null accessor bound to a buffer, is then bound to that buffer forever? can it re-assigned? can it be assigned back to null? What happens if two command groups submit a different require bounding the same null accessor to two separate buffers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main point of this MR is that the require function doesn't do the binding - it doesn't modify the accessor at all. So a null accessor can be "bound" by assigning to it a placeholder that already has an associated buffer.


|Member function |Description |
|---------------------|---------------------------------------------------------|
|bool is_null() const |Returns false if the accessor is associated with a buffer, true otherwise.|
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected output of is_null in the following cases?

Case 1:

accessor<T, dim, mode, target, access::placeholer::true_t> acc0;

q.submit([&](handler& h) {
      h.require(acc0, bufA);
});

// What is the output here?
std::cout << acc.is_null() << std::endl;

Case 2:

accessor<T, dim, mode, target, access::placeholer::true_t> acc0;

std::thread t([](){
  q.submit([&](handler& h) {
      h.require(acc0, bufA);
  });
});
// What is the output here?
std::cout << acc.is_null() << std::endl;
t.join();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR doesn't have that overload of the require function. The existing function doesn't modify the state of the accessor, and having an overload that would modify it is a dangerous inconsistency. That also means that both your cases would print true.

h.require(acc0, bufA);
// is instead
acc0 = accessor<T, dim, mode, target, access::placeholer::true_t>(bufA);
h.require(acc0);
// or
acc0 = make_placeholder_accessor<mode, target>(bufA);
h.require(acc0);

Copy link
Contributor

@Ruyk Ruyk left a comment

Choose a reason for hiding this comment

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

Its the missing piece on placeholder accessors, but needs a couple of clarification. The implicit state of the placeholder accessor requires some clarification.

@ProGTX
Copy link
Contributor Author

ProGTX commented Apr 17, 2020

This has been replaced by #124, which defines an accessor to be a Container, and then you can have "empty" accessors if acc.empty() == true.

@ProGTX ProGTX closed this Apr 17, 2020
@keryell
Copy link
Contributor

keryell commented Apr 18, 2020

This has been replaced by #124, which defines an accessor to be a Container, and then you can have "empty" accessors if acc.empty() == true.

Nice side effect of this container (point-of-) view...

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.

3 participants