[SYCL][RTC] Define custom destructor for kernel_bundle_impl
#16702
Conversation
… bundle is destroyed Signed-off-by: Julian Oppermann <[email protected]>
Signed-off-by: Julian Oppermann <[email protected]>
| RTDeviceBinaryImage *Img = *RawImages.begin(); | ||
|
|
||
| // Drop the kernel argument mask map | ||
| // TODO: Why is this not protected by a mutex? |
There was a problem hiding this comment.
Do we still need to address this TODO as part of this PR?
There was a problem hiding this comment.
What the TODO is asking is why m_EliminatedKernelArgMasks not protected by a mutex in the existing code, in particular in addImages which my method here tries to mirror. So this is potentially a separate issue, and I was hoping reviewers could explain the rationale here.
There was a problem hiding this comment.
Okay, maybe @sergey-semenov can provide some insight here.
There was a problem hiding this comment.
I don't see why it shouldn't be. seems like a bug to me.
There was a problem hiding this comment.
I filed #16836, and dropped the TODO from the code for now.
Signed-off-by: Julian Oppermann <[email protected]>
| @@ -927,6 +930,13 @@ class kernel_bundle_impl { | |||
| return true; | |||
| } | |||
|
|
|||
| ~kernel_bundle_impl() { | |||
| if (DeviceBinaries) { | |||
There was a problem hiding this comment.
removeImages() might throw an exception (because getSYCLKernelID and several other sub-calls can)
But ~kernel_bundle_impl() itself is implictly noexcept. So you'll need a try/catch here. You can use that __SYCL_REPORT_EXCEPTION_TO_STREAM macro if appropriate.
Signed-off-by: Julian Oppermann <[email protected]>
Signed-off-by: Julian Oppermann <[email protected]>
Signed-off-by: Julian Oppermann <[email protected]>
|
@sommerlukas could you re-review after 907f7f8, please? |
Signed-off-by: Julian Oppermann <[email protected]>
Signed-off-by: Julian Oppermann <[email protected]>
jopperm
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Julian Oppermann <[email protected]>
jopperm
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
jopperm
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers Please merge, thanks! |
Adds a custom destructor to
kernel_bundle_implto properly clean-up runtime information and device binaries for bundles that are runtime-compiled from source:Removal of kernels from program manager
The new
ProgramManager::removeImagesmethod takesaddImagesas a blueprint and reverses the effects of registering the given device binaries. Currently, only a subset of the data structures in the program manager is handled, andasserts are in place for the remaining members. Device globals will be addressed after [SYCL][RTC] Initial support for device globals #16565 lands. AFAICT I could clean-up most of the other members mechanically as well, but decided against that because I can't test these features (such as virtual functions) right now due to lack of SYCL-RTC support.Free device binaries in JIT library
Store the UR-specific data structure that backs a device binary in a map of unique ptrs instead of a vector to make it possible to free it again without invalidating other compilation results. Also introduce a mutex to protect concurrent access to this new map.
Free raw SPIR-V binaries in JIT context
Again replace a vector with a map of unique ptr to make it possible to free
KernelBinaryobjects in theJITContext.KernelBinaryowns the actual SPIR-V binaries; all other data structures mentioned earlier only store the pointer return byKernelBinary::address(). A new functiondestroyBinaryis added to thesycl-jitinterface.My understanding is that
~device_image_impland~kernel_implalready take care of releasing the underlying UR resources, hence no changes required there.