-
Notifications
You must be signed in to change notification settings - Fork 795
[SYCL][Doc] Add test plan for work_group_memory extension
#15639
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
Merged
AlexeySachkov
merged 2 commits into
intel:sycl
from
AlexeySachkov:private/asachkov/work-group-memory-test-plan
Oct 30, 2024
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| # Test plan for [`sycl_ext_oneapi_work_group_memory`][spec-link] extension | ||
|
|
||
| ## Testing scope | ||
|
|
||
| ### Device coverage | ||
|
|
||
| Functionality provided by the extension is not guarded by any aspects and | ||
| should be supported on every device supported by an implementation. | ||
|
|
||
| Therefore, those tests should be launched on every supported device | ||
| configuration that we have. | ||
|
|
||
| ### Type coverage | ||
|
|
||
| New APIs for allocating local memory described by the extension take data type | ||
| as a template argument and therefore tests should be repeated for different | ||
| data types to ensure that everything works correctly. | ||
|
|
||
| List of data types which should be covered: | ||
| - some basic C++ data types of different sizes: `char`, `int16_t`, `int`, | ||
| `double` | ||
| - `half` | ||
| - pointers | ||
| - user-defined types such as `struct`, `union` | ||
| - SYCL-provided data types such as `vec`, `marray` | ||
| - arrays (both bounded and unbounded) of the types listed above | ||
|
|
||
| **TODO**: the spec doesn't seem to disallow references and cv-qualified types to | ||
| be used with `work_group_memory` which is likely a bug in the spec. | ||
|
|
||
| **TODO**: it would not be possible or reasonable to repeat absolutely every test | ||
| with every data type. Should the specification of which tests should be repeated | ||
| for which data types be a part of the test plan, or should it be an | ||
| implementation detail of the said test plan? | ||
|
|
||
| ## Tests | ||
lbushi25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Unit tests | ||
|
|
||
| #### Interface tests | ||
|
|
||
| These tests are intended to check that all classes and methods defined by the | ||
| extension have correct implementation, i.e.: right signatures, right return | ||
| types, all necessary constraints are checked/enforced, etc. | ||
|
|
||
| Those tests are expected to be compile-only tests which doesn't require | ||
| execution of compiled code. | ||
|
|
||
| Things which we need to check here: | ||
|
|
||
| - that diagnostic is emitted if `PropertyListT` argument is set to anything | ||
| else than `empty_properties_t` | ||
| - that `value_type` type alias is properly defined | ||
| - that `work_group_memory` is default-constructible, copy-constructible and | ||
| copy-assignable | ||
| - that constructors accepting `handler` are also properly defined by the | ||
| implementation, including type restrictions they have | ||
| - that conversion operators are properly defined | ||
| - that `operator=(const DataT&)` is properly defined, including its type | ||
| restrictions | ||
| - that `get_multi_ptr` is properly defined and its `IsDecorated` template | ||
| argument is handled correctly | ||
| - that methods documented as `const` are indeed defined in the implementation | ||
| as `const` | ||
|
|
||
| Tests in this category may not perform some useful actions to exercise the | ||
| extension functionality in full, but instead they are focused on making sure | ||
| that all APIs are consistent with respect to other APIs. | ||
|
|
||
| #### Runtime diagnostics on misuse | ||
|
|
||
| `work_group_memory` objects can only be used in `parallel_for(nd_range)` | ||
| kernels, so the test should ensure that we throw a synchronious `exception` | ||
| with the `errc::kernel_argument` error code if `work_group_memory` was used by | ||
| other types of kernels (`single_task` and `parallel_for(range)`). | ||
|
|
||
| #### Consistency of address-taking operations | ||
|
|
||
| The test need to check that `operator&()` and `get_multi_ptr().get_raw()` both | ||
| return the same address. | ||
|
|
||
| #### Copy constructors and assignment operator | ||
|
|
||
| The test need to check that `operator&()` and `get_multi_ptr().get_raw()` both | ||
| return correct address after `work_group_memory` was copy-constructed, or | ||
| copy-assigned from another `work_group_memory` object. | ||
|
|
||
| #### Operations on different types | ||
|
|
||
| This test should check that an object of `work_group_memory` class behaves like | ||
| an object of a type `work_group_memory` is templated on, i.e. all operations | ||
| possible on an underlying type can also be performed on `work_group_memory` | ||
| object and produce correct results. | ||
|
|
||
| #### Number of kernel arguments produced by `work_group_memory` objects | ||
|
|
||
| One of the main reasons behind the extension is to provide a lightweight | ||
| alternative to `local_accessor`. To make sure that the implementation is indeed | ||
| lightweight, there should be a test which ensures that every `work_group_memory` | ||
| object results in a single kernel argument generated by the compiler. | ||
|
|
||
| ### End-to-end tests | ||
|
|
||
| Tests in this category perform some meaningful actions with the extension to | ||
| see that the extension works in a scenarios which mimic real-life usage of the | ||
| extension. | ||
|
|
||
| #### Basic usage | ||
|
|
||
| This test is intended to check that memory provided through `work_group_memory` | ||
| is indeed shared between work-items within a work-group and can be properly | ||
| accessed for reading-writing to it. | ||
|
|
||
| A test should allocate an array of integers through `work_group_memory` and | ||
| submit a single (for simplicity) work-group where every work-item would store | ||
| an element from a global buffer into that allocated local array which | ||
| corresponds to its local id. | ||
|
|
||
| Then, a single work-item should perform a reduction of that local array, i.e. | ||
| compute sum of its elements. The result should be communicated back to host and | ||
| verified there. | ||
|
|
||
| Some of the operations performed on `work_group_memory` object should be done in | ||
| a separate helper function to check that we can pass `work_group_memory` objects | ||
| to functions and correctcly access data they reference there. | ||
|
|
||
| #### Use of multiple `work_group_memory` objects | ||
|
|
||
| This test is similar to the one above, but a kernel should operate on more than | ||
| one `work_group_memory` object. For example, the second `work_group_memory` | ||
| object could be used to broadcast reduction value to all work-items in a | ||
| work-group. | ||
|
|
||
| #### Use of `work_group_memory` with `atomic_ref` | ||
AlexeySachkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| The test should check that `work_group_memory` objects could be used together | ||
| with `atomic_ref`: a scalar `work_group_memory` object could be used to | ||
| construct an `atomic_ref` object and the latter should be used to perform a | ||
| reduction (compute a sum, for exmple) of values in a global buffer. | ||
|
|
||
| #### Use of `work_group_memory` with free function kernel | ||
|
|
||
| One of important use cases which we anticipate is usage of `work_group_memory` | ||
| in kernels defined as free functions. | ||
|
|
||
| Basic usage test should be repeated with kernels defined as free functions. | ||
|
|
||
| [spec-link]: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_work_group_memory.asciidoc | ||
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.
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.
Is
doublepart of the list above or a separate type to be covered? Sincehalfis mentioned by itself, I think it would make sense to do that fordoubletoo. Should we also mention that the tests fordoubleandhalfrequireaspect::fp64andaspect::fp16respectively?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.
halfis in a separate list, because it is not a basic C++ type, its a custom type introduced by SYCL specification.I don't think that we should outline "optional" data types separately, because then we also need to consider
atomic_reffor 64-bit types and I generally expect that the fact that certain tests have "extra" requirements and may be skipped on certain HW as implied, simply because that is automatically the case if a test is properly written in accordance with the SYCL specification.