[SYCL][ESIMD] Support accessors for ESIMD kernels in SYCL RT. #1870
Conversation
| IMAGE_BUFFER_TY_DEFINE(read_write, rw); | ||
|
|
||
| template <> struct opencl_image1d_buffer_type<access::mode::atomic> { | ||
| // static_assert(false && "atomic access not supported for image1d |
There was a problem hiding this comment.
Probably you can have this static assert in the "default" definition of opencl_image1d_buffer_type on line 184 which should be instantiated if there is no matching specialization.
There was a problem hiding this comment.
It does not work unfortunately, as there is instantiation in stream_impl.hpp - GlobalOffsetAccessorT.
| @@ -1617,6 +1620,30 @@ static void ReverseRangeDimensionsForKernel(NDRDescT &NDR) { | |||
| } | |||
| } | |||
|
|
|||
| // Creates an image1d buffer wrapper object to enable surface index-based | |||
| // access for ESIMD. | |||
| static pi_mem createImage1dBufferWrapper(const context_impl &Ctx, size_t Size, | |||
There was a problem hiding this comment.
Conceptually this function should be in program_manager.cpp. Could you please move it there?
There was a problem hiding this comment.
do you mean memory manager?
There was a problem hiding this comment.
Yes, I meant memory manager
| if (IsESIMDAcc) { | ||
| // Create additional wrapper object for an ESIMD accessor and record | ||
| // it for subsequent de-allocation. | ||
| size_t AllocSize = Req->MMemoryRange.size() * Req->MElemSize; |
There was a problem hiding this comment.
Please move image allocation to AllocaCmd::enqueueImp.
There was a problem hiding this comment.
thanks, good catch
| // compiler option enabling the macro below. Eventually ESIMD kernels and usual | ||
| // kernels must co-exist and there must be a mechanism for distinguishing usual | ||
| // and ESIMD accessors. | ||
| #ifndef __SYCL_EXPLICIT_SIMD__ |
There was a problem hiding this comment.
This macro won't work because this file is a part of libsycl.so. Suggest adding ESIMD flag to AccessorImplHost and check it dynamically where needed.
There was a problem hiding this comment.
Right, thanks. This was a last-minute change for the open-source variant, could not really test it at this point.
| // Create additional wrapper object for an ESIMD accessor and record | ||
| // it for subsequent de-allocation. | ||
| size_t AllocSize = Req->MMemoryRange.size() * Req->MElemSize; | ||
| MemArg = createImage1dBufferWrapper(*getSyclObjImpl(Context), |
There was a problem hiding this comment.
Please, make sure that such combined alloca(buffer + image) works correctly in cases when we move memory between contexts.
There was a problem hiding this comment.
As agreed, I will add a TODO - ESIMD interop will need to be addressed.
There was a problem hiding this comment.
@s-kanaev, please review. I think I addressed all @romanovvlad's comments, except the dummy opencl_image1d_buffer_type specialization for atomic mode, which I can't find simple solution for at this point, and added a TODO.
There was a problem hiding this comment.
The code looks good to me, but some one else should take a look at that.
@sergey-semenov will you take a look?
sycl/source/detail/accessor_impl.cpp
Outdated
| @@ -37,6 +37,7 @@ void addHostAccessorAndWait(Requirement *Req) { | |||
| detail::Scheduler::getInstance().addHostAccessor(Req); | |||
| Event->wait(Event); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Minor. Unneeded change.
|
@bader, please take a look/merge. Change requested by @romanovvlad was implemented. |
|
@kbobrovs, please, resolve merge conflicts. |
1. Handle 1d accessors differently - wrap in image1d buffer object to work with ESIMD back-end. 2. Introduce new OpenCL image1d buffer type in device code - to represent accessor's wrapped memory object. 3. Add "private proxy" classes to access accessor's memory object in the simd library. Co-authored-by: Vlad Romanov <[email protected]> Signed-off-by: Konstantin S Bobrovsky <[email protected]>
|
@bader, done. Had to squash (and rebase) to avoid to-be-thrown-away resolution of conflicts on early commits. |
If I understand the explanation correctly, early commits could be reverted to avoid conflicts during the merge. @romanovvlad, please, approve. |
Some of the changes from early commits remained. So reverting would require more work on conflict resolution, potentially they could happen with all newer commits. |
I don't get this, but there is a way to avoid rebase in any case: revert all commits, merge, apply rebased/squashed commits on top of that. |
intel#1870 broke ABI by adding new field to AccessorImplHost. This patch improves ABI testing to catch this type of problems in future. Signed-off-by: Alexander Batashev <[email protected]>
#1870 broke ABI by adding new field to AccessorImplHost. This patch improves ABI testing to catch this type of problems in future. Signed-off-by: Alexander Batashev <[email protected]>
Main changes:
Specific tests will be added once other parts get merged (simd library specifically) which are used by the tests.