[SYCL][Doc] Add indeterminate to work_group_memory #15933
Merged
Conversation
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
We decided that it was too easy to mistakenly use the default
constructor like this, with the expectation that it statically
allocates work-group local memory:
```
void device_code() {
syclex::work_group_memory<int> mem;
}
```
To make this error less likely, we add a parameter to the constructor
named `indeterminate`:
```
void device_code() {
syclex::work_group_memory<int> mem{syclex::indeterminate};
}
```
We hope this will make it more apparent that `mem` is just a dummy
object, and it needs to be assigned to some other `work_group_memory`
object before it can be used.
Pennycook
approved these changes
Oct 31, 2024
There was a problem hiding this comment.
This suggestion is out of scope for this PR, but we might want to make some time to look at other extensions and see whether there are any that could benefit from this same approach.
During discussion on #15861 we spotted that accessor already has this same behavior, and I wouldn't be surprised if we found more cases.
|
@intel/llvm-gatekeepers I think this is ready to merge. |
This was referenced Nov 5, 2024
martygrant
pushed a commit
that referenced
this pull request
Nov 15, 2024
…16003) Implement the changes made to the work group memory spec in this PR: #15933. Essentially, the default constructor is removed from the spec and instead a constructor that takes an indeterminate argument is made available in order to emphasize that any operation on such a work group memory object is undefined behavior except for assigning to it another work group memory object. Because the frontend needs special types to have a default constructor, we make it private instead and when its time for the frontend to call it, simply override the access specifier temporarily.
7 tasks
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.
We decided that it was too easy to mistakenly use the default constructor like this, with the expectation that it statically allocates work-group local memory:
To make this error less likely, we add a parameter to the constructor named
indeterminate:We hope this will make it more apparent that
memis just a dummy object, and it needs to be assigned to some otherwork_group_memoryobject before it can be used.