Skip to content

[SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating #4983

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 9, 2021
Merged

[SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating #4983

merged 7 commits into from
Dec 9, 2021

Conversation

s-kanaev
Copy link
Contributor

No description provided.

[SYCL] Only call shutdown when DLL is being unloaded, not when proces…
e386efe 
…s is terminating

Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
@s-kanaev s-kanaev requested a review from a team as a code owner November 18, 2021 09:00
alexbatashev
alexbatashev reviewed Nov 18, 2021
View reviewed changes
cperkinsintel
cperkinsintel previously approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

as per the docs that were shared, this looks good to me.

Is there any test we can add to confirm?

@s-kanaev
Copy link
Contributor Author

Is there any test we can add to confirm?

I can't think of any proper test except for launch and finish some dummy application and employing windows sort of ltrace which seem to be complex for CI.

@romanovvlad
Copy link
Contributor

Is there any test we can add to confirm?

I can't think of any proper test except for launch and finish some dummy application and employing windows sort of ltrace which seem to be complex for CI.

Can't we check output of SYCL_PI_TRACE for cases when lpReserved is nullptr or non-nullptr?

alexbatashev
alexbatashev previously approved these changes Nov 22, 2021
View reviewed changes
@s-kanaev
Copy link
Contributor Author

s-kanaev commented Nov 22, 2021 •
edited by bader
Loading

Can't we check output of SYCL_PI_TRACE for cases when lpReserved is nullptr or non-nullptr?

This might be feasible within an OS-specific unit-test with explicit call to DllMain.

I'll add another commit here.

Sergey Kanaev added 3 commits November 23, 2021 17:35
Add unit-test
d3ce279 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Merge remote-tracking branch 'public/sycl' into fix-windows-shutdown2
d81b4cb
Fixes in the test along with some outputs for verbosity
9832e6e 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
@s-kanaev s-kanaev dismissed stale reviews from alexbatashev and cperkinsintel via 9832e6e December 2, 2021 12:41
Sergey Kanaev added 2 commits December 2, 2021 15:44
Make unit-test a no-op on non-windows platform
93744b3 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Stylistic change
3643090 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
@s-kanaev
Copy link
Contributor Author

s-kanaev commented Dec 2, 2021 •
edited
Loading

@cperkinsintel , @alexbatashev , I've added a unit-test. Please, review.

Stylistic change
c183ba7 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
@s-kanaev s-kanaev force-pushed the fix-windows-shutdown branch from faeb978 to c183ba7 Compare December 2, 2021 13:05
alexbatashev
alexbatashev approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM, comments are minor

sycl/unittests/windows/dllmain.cpp
extern "C" BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason,
LPVOID lpReserved);

static std::atomic<int> TearDownCalls{0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. I would add a static_assert to check that this atomic is trivially destructible.

@s-kanaev s-kanaev requested a review from bader December 7, 2021 15:47
@bader
Copy link
Contributor

bader commented Dec 7, 2021

@s-kanaev, I don't have anything to add here.

@romanovvlad romanovvlad merged commit 4df4655 into intel:sycl Dec 9, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 11, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into hip_cuda_builds
f7efebe 
* upstream/sycl: (725 commits)
  [SYCL] Translate ZE_RESULT_ERROR_INVALID_ARGUMENT error code from L0 RT (intel#5122)
  [SYCL][L0][Plugin] Call ZeCommandQueueCreate on demand (intel#5109)
  [SYCL] Switch to using blocking USM free for OpenCL GPU (intel#4928)
  [CI] Disable pack and upload steps (intel#5119)
  [SYCL] Disable submission of AssertInfoCopier for FPGA (intel#4780)
  [SYCL][SPIRV] Implement islessgreater with FOrdNotEqual instead (intel#5076)
  [SYCL] Fix typo in the name of the host-visible pool (intel#5073)
  [SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating (intel#4983)
  [SYCL][CUDA][PI] Fix infinite loop when parallel_for range exceeds INT_MAX (intel#5095)
  [SYCL] Translate out-of-memory error codes from L0 RT (intel#5107)
  [SYCL] Fix a few warnings during build scripts configuration (intel#5082)
  [SYCL] Fix amdgpu openmp test (intel#5103)
  [SYCL] [FPGA] Create experimental headers for FPGA latency control (intel#5066)
  [SYCL][CUDA] Don't enqueue an event wait on same CUDA stream (intel#5099)
  Remove PR disable template (intel#5102)
  [BuildBot]Uplift CPU/FPGAEMU RT version (intel#5078)
  [SYCL] Fix the test to not depend on a specific line. (intel#5092)
  [CI] Provide libclc targets to build and test (intel#5091)
  Fix build of `check-llvm-spirv` target after 8f8001a
  Force opt to use new pass manager in pr52289 test after c34d157
  ...
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 12, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into llvm_test_suite
ec072b1 
* upstream/sycl:
  [CI] Add container users to video group (intel#5101)
  [CI] More typo fixes in Nightly build (intel#5088)
  Revert "[CI] Disable pack and upload steps (intel#5119)" (intel#5125)
  [SYCL] Translate ZE_RESULT_ERROR_INVALID_ARGUMENT error code from L0 RT (intel#5122)
  [SYCL][L0][Plugin] Call ZeCommandQueueCreate on demand (intel#5109)
  [SYCL] Switch to using blocking USM free for OpenCL GPU (intel#4928)
  [CI] Disable pack and upload steps (intel#5119)
  [SYCL] Disable submission of AssertInfoCopier for FPGA (intel#4780)
  [SYCL][SPIRV] Implement islessgreater with FOrdNotEqual instead (intel#5076)
  [SYCL] Fix typo in the name of the host-visible pool (intel#5073)
  [SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating (intel#4983)
  [SYCL][CUDA][PI] Fix infinite loop when parallel_for range exceeds INT_MAX (intel#5095)
  [SYCL] Translate out-of-memory error codes from L0 RT (intel#5107)
  [SYCL] Fix a few warnings during build scripts configuration (intel#5082)
  [SYCL] Fix amdgpu openmp test (intel#5103)
  [SYCL] [FPGA] Create experimental headers for FPGA latency control (intel#5066)
  [SYCL][CUDA] Don't enqueue an event wait on same CUDA stream (intel#5099)
  Remove PR disable template (intel#5102)
  [BuildBot]Uplift CPU/FPGAEMU RT version (intel#5078)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexbatashev alexbatashev alexbatashev approved these changes

@jandres742 jandres742 jandres742 left review comments

@GarveyJoe GarveyJoe GarveyJoe left review comments

@sherry-yuan sherry-yuan sherry-yuan left review comments

@cperkinsintel cperkinsintel Awaiting requested review from cperkinsintel

@bader bader Awaiting requested review from bader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@s-kanaev @romanovvlad @bader @alexbatashev @cperkinsintel @jandres742 @GarveyJoe @sherry-yuan