-
Notifications
You must be signed in to change notification settings - Fork 17
CP024 Default placeholders #122
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
Open
ProGTX
wants to merge
4
commits into
codeplaysoftware:master
Choose a base branch
from
ProGTX:ProGTX/default-placeholders
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,8 @@ | |
| |-------------|--------| | ||
| | Name | Default placeholders | | ||
| | Date of Creation | 9 March 2019 | | ||
| | Revision | 0.1 | | ||
| | Latest Update | 9 March 2020 | | ||
| | Revision | 0.2 | | ||
| | Latest Update | 17 March 2020 | | ||
| | Target | SYCL Next (after 1.2.1) | | ||
| | Current Status | _Work in Progress_ | | ||
| | Reply-to | Peter Žužek <[email protected]> | | ||
|
|
@@ -48,7 +48,7 @@ but hasn't been registered with a command group | |
| is more like a fancy pointer: | ||
| the user doesn't own the data | ||
| until the accessor is registered and used in a kernel, | ||
| where is becomes more similar to a regular pointer. | ||
| where it becomes more similar to a regular pointer. | ||
|
|
||
| Having this type separation between full accessors and placeholders | ||
| might be useful from a type safety perspective, | ||
|
|
@@ -123,11 +123,43 @@ class handler { | |
| }; | ||
| ``` | ||
|
|
||
| `handler::require` has to be called on a placeholder accessor | ||
| in order to register it with the command group submission. | ||
| It is valid to call the function more than once, | ||
| even on non-placeholder accessors. | ||
| Calling the function on a null accessor throws `cl::sycl::invalid_object_error`. | ||
|
|
||
| ### Deprecate `is_placeholder` | ||
|
|
||
| The function `accessor::is_placeholder` doesn't make sense anymore, | ||
| we propose deprecating it. | ||
|
|
||
| ### Allow constructing host accessors from placeholders | ||
|
|
||
| Consider the following example: | ||
|
|
||
| ```cpp | ||
| template<typename AccTypeA, typename AccTypeB> | ||
| void some_library_function(AccTypeA accA, AccTypeB accB) { | ||
| ... | ||
| myQueue.submit([&](handler &cgh) { | ||
| cgh.require(accA); | ||
| cgh.require(accB); | ||
| cgh.copy(accA, accB); | ||
| }); | ||
| ... | ||
| // We want to be able to access host data now | ||
| } | ||
| ``` | ||
|
|
||
| `some_library_function` in the example takes in two placeholder accessors | ||
| and performs a copy from one to another. | ||
| However, there is no way any of the data associated with the accessors | ||
| can be accessed on the host. | ||
| The placeholders are not bound to a command group anyway, | ||
| so we believe it should be possible to explicitly construct a host accessor | ||
| from a placeholder accessor. | ||
|
|
||
| ### New constructors | ||
|
|
||
| We propose adding new constructors to the accessors class | ||
|
|
@@ -144,6 +176,17 @@ to allow placeholder construction. | |
| normally an accessor constructor requires a buffer and a handler, | ||
| we propose making the handler optional. | ||
| This is the same constructor currently allowed for host buffers. | ||
| 1. Construct a host accessor from a placeholder one (`placeholderAcc`). | ||
| Not valid to call in kernel code. | ||
| Throws `cl::sycl::runtime_error` when called | ||
| if `placeholderAcc.has_handler() == true`. | ||
| Requesting host access is a synchronization point, | ||
| and host accessors act as locks, | ||
| meaning that the placeholder cannot be used | ||
| while the host accessor is in scope. | ||
| Even after host access is released, | ||
| the programmer is required to call `require` again on the placeholder | ||
| before it can be used in a kernel. | ||
|
|
||
| ```cpp | ||
| template <typename dataT, | ||
|
|
@@ -162,19 +205,29 @@ class accessor { | |
|
|
||
| // 2 | ||
| // Only available when: ((accessTarget == access::target::global_buffer) || | ||
ProGTX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // (accessTarget == access::target::constant_buffer) || | ||
| // (accessTarget == access::target::host_buffer)) && | ||
| // (accessTarget == access::target::constant_buffer)) && | ||
| // (dimensions == 0) | ||
| accessor(buffer<dataT, 1> &bufferRef); | ||
|
|
||
| // 3 | ||
| // Only available when: ((accessTarget == access::target::global_buffer) || | ||
| // (accessTarget == access::target::constant_buffer) || | ||
| // (accessTarget == access::target::host_buffer)) && | ||
| // (accessTarget == access::target::constant_buffer)) && | ||
| // (dimensions > 0) | ||
| accessor(buffer<dataT, dimensions> &bufferRef, | ||
| range<dimensions> accessRange, | ||
| id<dimensions> accessOffset = {}); | ||
|
|
||
| // 4 | ||
| // Only available when (accessTarget == access::target::host_buffer) && | ||
| // ((otherTarget == access::target::global_buffer) || | ||
| // (otherTarget == access::target::constant_buffer)) | ||
| template <access::target otherTarget, access::placeholder otherPlaceholder> | ||
| accessor(accessor<dataT, | ||
ProGTX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| dimensions, | ||
| accessMode, | ||
| otherTarget, | ||
| otherPlaceholder>& | ||
| placeholderAcc); | ||
| }; | ||
| ``` | ||
|
|
||
|
|
@@ -186,10 +239,16 @@ we propose new member functions to the `accessor class`: | |
| 1. `is_null` - returns `true` if the accessor has been default constructed, | ||
ProGTX marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| which is only possible with placeholders. | ||
| Not having an associated buffer is analogous to a null pointer. | ||
| Available in both application code and kernel code, | ||
| it is valid to pass a null accessor to a kernel. | ||
| 1. `has_handler` - returns `true` if the accessor is associated | ||
ProGTX marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| with a command group `handler`. | ||
| Will only be `false` with host accessors and placeholder accessors. | ||
| This replaces the `is_placeholder` member function. | ||
| Mainly meant as a way to enquire about whether this is a placeholder or not, | ||
| this doesn't have to be checked before `require` is called. | ||
| 1. `get_host_access` - constructs a host accessor from a placeholder accessor. | ||
| Not valid to call in kernel code. | ||
|
|
||
| ```cpp | ||
| template <typename dataT, | ||
|
|
@@ -206,6 +265,16 @@ class accessor { | |
|
|
||
| // 2 | ||
| bool has_handler() const noexcept; | ||
|
|
||
| // 3 | ||
| // Only available when ((accessTarget == access::target::global_buffer) || | ||
| // (accessTarget == access::target::constant_buffer)) | ||
| accessor<dataT, | ||
| dimensions, | ||
| accessMode, | ||
| access::target::host_buffer, | ||
| access::placeholder::false_t> | ||
| get_host_access() const; | ||
| }; | ||
| ``` | ||
|
|
||
|
|
@@ -256,7 +325,6 @@ myQueue.submit([&](handler &cgh) { | |
| }); | ||
|
|
||
| // Submit kernel that writes to output buffer | ||
| // Use constant buffer accessors | ||
| buffer<int> bufC{bufRange}; | ||
| accC = read_acc{bufC}; | ||
| myQueue.submit([&](handler &cgh) { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
We may also need to make a tweak to the
buffer::get_accessvariant which takes no parameters, as that's currently defined to always return anaccessorofaccess::target::host_buffer. If we want to be able to retrieve a placeholderaccessorusing this member function as well, we will need to disambiguate it. One option could be to require that theaccess::targetbe specified, another could be to have a different member function for this. Alternatively, we could just rely on theaccessorconstructor for this, asget_accessis simply syntactic sugar over the 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'm not sure
get_accessshould return a placeholder, it's probably better to always return a full accessor. In terms of verbosity, #100 would probably be sufficient for resolving that.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 this is a good approach, this means that
get_accessis the full contract of associating theaccessorwith abufferandhandler, and if you want to create anaccessorfrom abufferwithout associating ahandlerwith it you just use the constructor.