Skip to content
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] Remove UR objects release checks on Windows #15425

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Sep 18, 2024

These e2e tests started failing after PI removal and replacing it with UR (PR) However, they were not related to it.

On Windows, dlls unloading is inconsistent and if we try to release these UR objects manually, inconsistent hangs happen due to a race between unloading the UR adapters dlls (in addition to their dependency dlls) and the releasing of these UR objects (The proxy loader have solved this problem partially here). So, we currently shutdown without releasing them and windows should handle the memory cleanup.

This behaviour is the same old behaviour as before removing PI as on Investigations on this. This was only hidden before PI removal as it was calling the PI entry-point but doesn't make it to UR entry-point and Filecheck logs check for objects release would pass as it only check for the call to the PI entry-point without check that the call was a successful call. That was the PI call for piContextRelease before PI removal:

---> piContextRelease(
        <unknown> : 0000023CC0EBA6C0
) ---> API Called After Plugin Teardown, Functon Call ignored.

Fixes #14768
Fixes #14950
Fixes #14968

@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 55b2a9b to c1050cc Compare September 19, 2024 10:34
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch 2 times, most recently from ccf6a4c to bcec694 Compare September 24, 2024 12:59
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from bcec694 to a429332 Compare September 24, 2024 13:22
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from a429332 to 96d1736 Compare September 25, 2024 10:41
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 96d1736 to 0d1702e Compare September 25, 2024 14:14
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 0d1702e to 091fea9 Compare September 26, 2024 10:13
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 091fea9 to bd66098 Compare September 26, 2024 14:51
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from bd66098 to 955e38d Compare September 26, 2024 15:03
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 955e38d to 975aca9 Compare September 26, 2024 15:36
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch 3 times, most recently from 2822cfa to c1f4489 Compare October 3, 2024 16:02
@omarahmed1111 omarahmed1111 changed the title [SYCL] Release resources on windows on shutdown [SYCL] Remove UR objects release checks on Windows Oct 4, 2024
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from c1f4489 to 0f5894c Compare October 4, 2024 14:43
@omarahmed1111 omarahmed1111 force-pushed the release-resources-on-windows-shutdown branch from 0f5894c to 63a28ff Compare October 4, 2024 16:10
@omarahmed1111 omarahmed1111 marked this pull request as ready for review October 4, 2024 16:11
@omarahmed1111 omarahmed1111 requested a review from a team as a code owner October 4, 2024 16:11
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits, otherwise LGTM.

sycl/test-e2e/Basic/queue/release.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Basic/queue/release.cpp Outdated Show resolved Hide resolved
@maarquitos14
Copy link
Contributor

Please, don't force-push after reviews started, as it makes it difficult for reviewers to know what changes are new and what changes are already reviewed.

@omarahmed1111
Copy link
Contributor Author

@intel/llvm-gatekeepers Please merge, Thanks!

@steffenlarsen steffenlarsen merged commit 006dc42 into intel:sycl Oct 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants