[SYCL] Extend image cleanup for __sycl_unregister_lib #17091
+610
−138
Conversation
This file contains 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
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
…ion that is not the case for removeImage usage and to be removed Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
KseniyaTikhomirova
changed the title
|
@jopperm hi, we overlapped in code a bit, I was fixing bugs of dynamic images load/unload. I removed duplications and added some extra details. I saw that you didn't cleaned up containers that you can't test with E2E test now - I added unit test where I can check them. |
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
jopperm
approved these changes
Feb 23, 2025
| @@ -3594,6 +3629,8 @@ extern "C" void __sycl_register_lib(sycl_device_binaries desc) { | |||
|
|
|||
| // Executed as a part of current module's (.exe, .dll) static initialization | |||
| extern "C" void __sycl_unregister_lib(sycl_device_binaries desc) { | |||
| (void)desc; | |||
| // TODO implement the function | |||
| // No need in fair partial cleanup at shutdown | |||
There was a problem hiding this comment.
Suggested change
| // No need in fair partial cleanup at shutdown | |
| // Partial cleanup is not necessary at shutdown |
There was a problem hiding this comment.
done in 42a642f
| assert(It != m_KernelName2KernelIDs.end()); | ||
| m_KernelName2KernelIDs.erase(It); | ||
| m_KernelIDs2BinImage.erase(It->second); | ||
| // remove Everything associated with this KernelName |
There was a problem hiding this comment.
Suggested change
| // remove Everything associated with this KernelName | |
| // remove everything associated with this KernelName |
There was a problem hiding this comment.
done in 42a642f
Comment on lines
2038
to
2039
| std::ignore = m_KernelUsesAssert.erase(EntriesIt->name); | ||
| std::ignore = m_KernelImplicitLocalArgPos.erase(EntriesIt->name); |
There was a problem hiding this comment.
Do we need to std::ignore the return values? There are plenty of other erase calls where we don't.
There was a problem hiding this comment.
To be honest I do not know if our code analyzer reacts on unhandled return value in this case. I removed them. It is not a big deal to add them later if needed.
42a642f
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
@uditagarwal97 FYI I don't add extra test for kernel cache update since I reused kernel eviction related code a lot. I rely on unit tests for kernel eviction and E2E test that failed because of absence of cache cleanup in dynamic image loading case - ./sycl/test-e2e/syclcompat/kernel/kernel_lin.cpp. |
uditagarwal97
approved these changes
Feb 24, 2025
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
KseniyaTikhomirova
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KseniyaTikhomirova
requested a deployment
to
WindowsCILock
GitHub Actions
Queued
uditagarwal97
approved these changes
Feb 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #16031