-
Notifications
You must be signed in to change notification settings - Fork 787
[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
[SYCL] Windows Proxy Loader for DLLs #8242
Conversation
… 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.
// accidentally retain device handles. etc | ||
void shutdown(){ | ||
GlobalHandler *&Handler = GlobalHandler::getInstancePtr(); | ||
Handler->unloadPlugins(); |
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 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.
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.
I added a comment to each definition of piTearDown in the plugin libs.
Why is this Windows specific? I thought similar problem with the order of unloads exists on Linux. |
@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 ). |
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 overall. But would like to let @sergey-semenov and @smaslov-intel approve.
LGTM overall, just some minor non-blocking comments. |
ping to reviewers |
/verify with intel/llvm-test-suite#1465 |
ping to @intel/dpcpp-esimd-reviewers and @intel/dpcpp-l0-pi-reviewers code owners |
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.
pi_esimd_emulator.cpp LGTM
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