[SYCL] default context as default on windows #12599
Conversation
…t work well on Windows with the default context enabled.
cperkinsintel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
cperkinsintel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
changed the title
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
…ced it when resolving merge conflicts. Removing it from this PR
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| #else | ||
| // Windows does not maintain dependencies between dynamically loaded libraries | ||
| // and can unload SYCL runtime dependencies before sycl.dll's DllMain has | ||
| // finished. To avoid calls to nowhere, intentionally leak platform to device | ||
| // cache. This will prevent destructors from being called, thus no PI cleanup | ||
| // routines will be called in the end. | ||
| // Update: the pi_win_proxy_loader addresses this for SYCL's own dependencies, | ||
| // but the GPU device dlls seem to manually load yet another DLL which may | ||
| // have been released when this function is called. So we still release() and | ||
| // leak until that is addressed. context destructs fine on CPU device. | ||
| MPlatformToDefaultContextCache.Inst.release(); | ||
| #endif |
There was a problem hiding this comment.
Do I get it right that you have to modify all the tests because this workaround is removed? Do we have to remove it to make default context as default?
There was a problem hiding this comment.
This workaround (or not) has no bearing on the leak check itself. If Windows uses a default context, the leak check races and may report leaks. If Windows does not use a default context, it doesn't.
But the call here to .release() vs. .reset() DOES determine whether the default context (if one exists) ACTUALLY leaks or not. We want .reset() so that it is actually destroyed, otherwise it leaks.
And we want the behavior Lin vs Win to the same ( as much as we can manage ) so it is easier to think about.
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/sycl-graphs-reviewers - ping for review. This touches the //RUN directive of many of your tests, but just changing the LEAK check to a placeholder. |
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
cperkinsintel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Now that there have been improvements to Windows shutdown made by the proxy loader, host task handling, and the unified runtime, we are finally able to have Windows support a "default" context. This makes any routine that loops over queue creation, etc perform much faster.
Note that on Windows the UR L0 Leak check does not work correctly with the default contexts on Windows. This is because on Windows the release of the plugin DLLs races against the release of static global variables (like the default context). This has been reported.
The tests that use this feature have been modified to use a new
%{l0_leak_check}substitution that will NOT use default context on Windows.