-
Notifications
You must be signed in to change notification settings - Fork 770
[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517
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
[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517
Conversation
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
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.
LGTM. Would it be possible to also add a test covering these changes?
both objects are static so not sure if it is possible to track with UT |
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.
Makes sense, but please add a PR description and consider updating the title, which is a bit misleading because jit_compiler
doesn't load ocloc
but rather the sycl-jit
library.
sycl/source/detail/jit_compiler.hpp
Outdated
@@ -98,6 +98,7 @@ class jit_compiler { | |||
CompileSYCLFuncT CompileSYCLHandle = nullptr; | |||
ResetConfigFuncT ResetConfigHandle = nullptr; | |||
AddToConfigFuncT AddToConfigHandle = nullptr; | |||
void *MLibraryHandle = nullptr; |
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.
Could this be a unique pointer as well, reusing the custom deleter from the newly-added unique pointer in jit_compiler
's constructor? Then we wouldn't need to duplicate the unload logic in the destructor.
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.
Nit: The other members here don't carry the M
prefix.
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.
done in c86ef55
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.
title is slightly updated too.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
@intel/llvm-gatekeepers hi, this PR is ready to be merged |
Fixes memory leaks in jit_compiler and kernel_compiler_opencl classes.
Libraries loaded to provide compilation utils have to be released.