[SYCL] Queue shortcut functions with placeholder accessors #7266
Conversation
These fulfil the basic spec as outlined in the description column of Table 29: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:queue-shortcuts
Seem like erroneous additions as manually invoking clang-format-diff doesn't re-add them. So removing them.
- Still debating how to test ::require and accessors - Debating whether or not to split into separate tests with setup.
Unit testing was initially chosen but it became apparent that we weren't able to test the handler::require call due to its inability to be mocked or replaced in the queue, so it was changed to an end-to-end test instead.
Still passes tests!
| /// Copies data from a memory region pointed to by a placeholder accessor to | ||
| /// another memory region pointed to by a shared_ptr. | ||
| /// | ||
| /// \param Src is a placeholder accessor to the source memory. |
There was a problem hiding this comment.
Not sure the API works with placeholder accessors only, does it?
There was a problem hiding this comment.
Technically it should be as this is a function on queue, so at this level it should only be possible to have placeholder and host accessors, the latter being moved to host_accessor. We could have a check to make sure it is not a host accessor in the meantime.
Can we check that PI APIs with correct parameters are called for each shortcut queue API? |
Yes this is possible. I will reintroduce the unit test with the mocked PI API functions to check the calls. |
update_host doesn't call any so could probably be removed. Actual verification of results is performed in llvm-test-suite.
|
This unit test has now been added. |
| unittest::PiMock Mock; | ||
| platform Plt = Mock.getPlatform(); | ||
|
|
||
| Mock.redefine<detail::PiApiKind::piEnqueueMemBufferWrite>( |
There was a problem hiding this comment.
Minor. It's not critical in this specific case, but it would be better to use redefineAfter or redefineBefore to let the original mock function execute.
| accessor<int, 1, access::mode::write, access::target::device, | ||
| access::placeholder::true_t, ext::oneapi::accessor_property_list<>> |
There was a problem hiding this comment.
I think SYCL 2020 makes access::placeholder::true_t unnecessary. I'd suggest to either simplify or at least add as extra simple
accessor Dest(Buf, sycl::write);
test.
Implementation of the explicit copy-based queue shortcut functions defined in Table 29 of Section 4.6.5.2 of the specification (the last 7 rows of the table to be precise).
An end-to-end test was used in place of a unit test as it was not possible to verify or mock the functionality of the
handler::requirefunction - everything else was testable withPIMock.Each shortcut function is implemented in accordance with its description in the table, and I've added doxygen comments to each to document parameter and behaviour information as with the rest of the functions in
queue.hpp.