Skip to content

[SYCL] Windows Proxy Loader for DLLs #8242

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

Merged

Conversation

cperkinsintel
Copy link
Contributor

@cperkinsintel cperkinsintel commented Feb 8, 2023

DLLs manually loaded by SYCL are not tracked as direct dependencies in the same way that linked DLLs are. This means these DLL may be unloaded before SYCLs shutdown() routine is called, which will lead to problems when that routine tries to call those DLL to release resources. This PR adds a new proxy DLL that is a linked dependency of SYCL itself. This proxy DLL loads all the SYCL manually loaded DLLs early, before SYCL itself is loaded, and conversely, they are not unloaded until the proxy itself unloads, which is after SYCL unloads. So now the manually loaded plugin DLLs will be resident when shutdown() is called and piTearDown can complete safely and successfully.

I had a previous PR for this work ( #7756 ), but it encountered interference with a difference in how Windows handles threads and their termination. I'm addressing that problem separately. In this version, I am reducing the shutdown() procedure on Windows to only release the plugins and nothing else. This avoids the issue for now.

Tests are at intel/llvm-test-suite#1465

… the same way that llinked DLLs are. This means these DLL may be unloaded before SYCLs shutdown() routine is called, which will lead to problems when that routine tries to call those DLL to release resources. This PR adds a new proxy DLL that is a linked dependency of SYCL itself. This proxy DLL loads all the SYCL manually loaded DLLs early, before SYCL itself is loaded, and conversely, they are not unloaded until the proxy itself unloads, which is after SYCL unloads. So now the manually loaded plugin DLLs will be resident when shutdown() is called and piTearDown can complete safely and successfully.

Signed-off-by: Chris Perkins <[email protected]>
…minated all the obvious candidates, but no luck. No choice but to return to not calling shutdown() when using XPTI.
@cperkinsintel cperkinsintel temporarily deployed to aws February 8, 2023 02:10 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 9, 2023 01:31 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 10, 2023 19:51 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 11, 2023 08:07 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 13, 2023 21:44 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 13, 2023 22:48 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel marked this pull request as ready for review February 14, 2023 17:42
@cperkinsintel cperkinsintel requested review from a team as code owners February 14, 2023 17:42
@cperkinsintel cperkinsintel changed the title [TEST] win proxy loader revisited [SYCL] Windows Proxy Loader for DLLs Feb 14, 2023
// accidentally retain device handles. etc
void shutdown(){
GlobalHandler *&Handler = GlobalHandler::getInstancePtr();
Handler->unloadPlugins();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a comment saying that piTearDown might not be safe to call low level API, since there might be dependent libraries that are unloaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to each definition of piTearDown in the plugin libs.

@smaslov-intel
Copy link
Contributor

DLLs manually loaded by SYCL are not tracked as direct dependencies in the same way that linked DLLs are.

Why is this Windows specific? I thought similar problem with the order of unloads exists on Linux.

@cperkinsintel
Copy link
Contributor Author

@smaslov-intel - no, it is Windows specific. DLLs that are dynamically loaded are not tracked as dependencies of the caller, and can be unloaded before the caller itself is done, which leads to problems for us if our shutdown procedures call into the plugins, or if the users application holds static variable to sycl constructs ( like sycl::queue or sycl::context ).
On Linux we see no issues unloading SYCL RT , the entire shutdown() procedure runs there with no trouble.

@cperkinsintel cperkinsintel temporarily deployed to aws February 17, 2023 20:25 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 19, 2023 12:24 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 21, 2023 07:25 — with GitHub Actions Inactive
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

LGTM overall. But would like to let @sergey-semenov and @smaslov-intel approve.

@cperkinsintel cperkinsintel temporarily deployed to aws February 21, 2023 12:04 — with GitHub Actions Inactive
@sergey-semenov
Copy link
Contributor

LGTM overall, just some minor non-blocking comments.

@cperkinsintel cperkinsintel temporarily deployed to aws February 22, 2023 21:09 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws February 22, 2023 21:41 — with GitHub Actions Inactive
@cperkinsintel
Copy link
Contributor Author

ping to reviewers

@cperkinsintel
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1465

@cperkinsintel
Copy link
Contributor Author

ping to @intel/dpcpp-esimd-reviewers and @intel/dpcpp-l0-pi-reviewers code owners

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

pi_esimd_emulator.cpp LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants