[SYCL] Add explicit update API to graph spec #77
Closed
Conversation
This file contains 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
- Add an explicit update API to the graph specification.
Bensuo
changed the title
EwanC
reviewed
Feb 13, 2023
EwanC
approved these changes
Mar 7, 2023
reble
reviewed
Mar 7, 2023
Since the individual update function is only applicable to buffers and USM allocations, rename it `update_memory_parameter()` and elaborate on why it isn't possible to update scalar parameters.
Bensuo
added
the
Spec Revision 2
julianmi
reviewed
Apr 18, 2023
| Exceptions: | ||
|
|
||
| * Throws synchronously with error code `invalid` if `from` is not found within the graph | ||
| or if `to` is not compatible with `from`. |
There was a problem hiding this comment.
Do we need to specify what incompatible entails here? I feel like this statement is very generic.
|
This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days. |
|
Closing in favour of #340 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on feedback from the specification PR. Originally we had argued for a more detailed API where the user would get information about parameters and then set them on the graph. This is similar to how things are done in CUDA but when actually developing the API in that direction it became clear there were numerous issues:
syclMemObjTwhich carries no information other than the size of the buffer. CUDA deals purely in pointers so this is less of an issue.The original suggestion on the specification PR is seen in this PR as it avoids the user having to book keep (other than to keep the original parameters around).
I don't believe supporting updating scalar parameters captured by SYCL kernels is possible here though. Since ultimately the type information is lost and we will almost certainly need to take a copy of the scalar data in the implementation in future we would have no basis for comparison. However, since these parameters are considered to be captured by value in command group scope and are not a formal concept in SYCL, I don't believe this is an issue.
I've marked this PR as a draft for now to facilitate discussion about the approach taken.