Skip to content

[SYCL] Fixes circular reference between events and queues #1226

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 1 commit into from
Mar 4, 2020
Merged

[SYCL] Fixes circular reference between events and queues #1226

merged 1 commit into from
Mar 4, 2020

Conversation

steffenlarsen
Copy link
Contributor

There is a potential circular reference between queues and events as a queue can hold a collection of shared pointers to events and events may have a shared pointer to a queue. If a circular reference is created, the only way to currently break the circle is to call wait on the queue, which is not ensured to happen.

To mitigate this, this PR changes makes the reference from an event to a queue a weak pointer. Effectively this will mean that if all remaining references to a queue are from events the queue will die. This affects only an events ability to throw asynchronous errors through the async_handler of the queue if it is destroyed prematurely.

Signed-off-by: Steffen Larsen [email protected]

@bader bader requested review from sergey-semenov and alexbatashev and removed request for sergey-semenov March 2, 2020 11:47
alexbatashev
alexbatashev previously approved these changes Mar 3, 2020
View reviewed changes
alexbatashev
alexbatashev reviewed Mar 3, 2020
View reviewed changes
@steffenlarsen steffenlarsen force-pushed the steffen/fix-event-queue-circ-refs branch from 4895c2c to f40bd4a Compare March 3, 2020 10:57
sergey-semenov
sergey-semenov approved these changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM

@romanovvlad
Copy link
Contributor

I think instead of dealing with circular dependencies here we should just remove queue from event. And replace the only use of queue - call of queue::throw_asynchronous with call to similar function but in context like stated in the SYCL specification instead.

@sergey-semenov
Copy link
Contributor

I think instead of dealing with circular dependencies here we should just remove queue from event. And replace the only use of queue - call of queue::throw_asynchronous with call to similar function but in context like stated in the SYCL specification instead.

I 100% agree with that, but I'm also fine with this patch as a solution for the immediate problem of circular dependency.

@bader
Copy link
Contributor

bader commented Mar 3, 2020 •
edited
Loading

I think instead of dealing with circular dependencies here we should just remove queue from event. And replace the only use of queue - call of queue::throw_asynchronous with call to similar function but in context like stated in the SYCL specification instead.

I 100% agree with that, but I'm also fine with this patch as a solution for the immediate problem of circular dependency.

So, how difficult it is to implement Vlad's proposal? I would like to avoid re-doing the work here.

@sergey-semenov
Copy link
Contributor

So, how difficult it is to implement Vlad's proposal? I would like to avoid re-doing the work here.

Well, our current implementation isn't capable of associating asynchronous errors with contexts directly, all async error handling is done on a per-queue basis. So, relatively speaking, it's going to be a much broader change than this one.

@bader bader merged commit 8c71dcb into intel:sycl Mar 4, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 4, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/refa…
3749455 
…ctor_tests

* origin/sycl: (32 commits)
  [SYCL] Fix circular reference between events and queues (intel#1226)
  [CI][Doc] Use SSH to deploy GitHub Pages (intel#1232)
  [SYCL][CUDA][Test] Testing for use of CUDA primary context (intel#1174)
  [SYCL] allow underscore symbol in temporary directory name
  [SYCL] Reject zero length arrays (intel#1153)
  [SYCL] Fix static code analyzis concerns (intel#1189)
  [SYCL] Add more details about the -fintelfpga option (intel#1218)
  [SYCL][CUDA] Select only NVPTX64 device binaries (intel#1223)
  [SYCL] Reverse max work-group size order (intel#1177)
  [SYCL][Doc] Add GroupAlgorithms extension (intel#1079)
  [SYCL] Fix SYCL internal enumerators conflict with user defined macro (intel#1188)
  [SYCL][CUDA] Fixes context release and unnamed context scope (intel#1207)
  [SYCL][CUDA] Fix context creation property parsing
  [CUDA][PI] clang-format pi.h
  [SYCL][CUDA] Handle the case of not having any CUDA device (intel#1212)
  [SYCL] Fix check-sycl-deploy target problems (intel#1165)
  [SYCL] Disable tests which take more than 5 minutes (intel#1220)
  [SYCL] Make context constructors explicit to avoid unintended conversions (intel#1219)
  [SYCL][NFC] Add clang-format configuration file for SYCL LIT tests (intel#1224)
  [SYCL] Fix command cleanup invoked from multiple threads (intel#1214)
  ...
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 5, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/host…
71cbcf6 
…_accessor_refactor

* origin/sycl: (38 commits)
  [SYCL] Fix device::get_devices() with a non-host device type (intel#1235)
  [SYCL][PI][CUDA] Implement kernel and kernel-group information queries (intel#1180)
  [SYCL] Remove default error code value in exception (intel#1150)
  [SYCL] Fix devicelib assert LIT test (intel#1245)
  [SYCL] Set aux-target-cpu for SYCL offload device compilation (intel#1225)
  [SYCL] Remove fabs and ceil from the list of unsupported math functions (intel#1217)
  [SYCL] Fix circular reference between events and queues (intel#1226)
  [CI][Doc] Use SSH to deploy GitHub Pages (intel#1232)
  [SYCL][CUDA][Test] Testing for use of CUDA primary context (intel#1174)
  [SYCL] allow underscore symbol in temporary directory name
  [SYCL] Reject zero length arrays (intel#1153)
  [SYCL] Fix static code analyzis concerns (intel#1189)
  [SYCL] Add more details about the -fintelfpga option (intel#1218)
  [SYCL][CUDA] Select only NVPTX64 device binaries (intel#1223)
  [SYCL] Reverse max work-group size order (intel#1177)
  [SYCL][Doc] Add GroupAlgorithms extension (intel#1079)
  [SYCL] Fix SYCL internal enumerators conflict with user defined macro (intel#1188)
  [SYCL][CUDA] Fixes context release and unnamed context scope (intel#1207)
  [SYCL][CUDA] Fix context creation property parsing
  [CUDA][PI] clang-format pi.h
  ...
KarachunIvan pushed a commit to KarachunIvan/llvm that referenced this pull request Sep 4, 2020
[SYCL] Eliminated events dependency on queue.
b83b7d3 
This patch removes events dependency on queue according to
suggestions from intel#1226.

Signed-off-by: Ivan Karachun <[email protected]>
@KarachunIvan KarachunIvan mentioned this pull request Sep 4, 2020
Merged
bader pushed a commit that referenced this pull request Sep 4, 2020
[SYCL] Eliminated events dependency on queue. (#2378)
31843cc 
This patch removes events dependency on queue according to
suggestions from #1226.

Signed-off-by: Ivan Karachun <[email protected]>
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023
@dongkyunahn-intel
[SYCL][ESIMD][EMU] Set default platform for ESIMD_EMULATOR (intel#1226)
14a7864 
* [SYCL][ESIMD][EMU] Setting default platform for ESIMD_EMULATOR

- CM_EMU library package is recently updated to support multiple
targets. This fix is to avoid target platform setting failure
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

@sergey-semenov sergey-semenov sergey-semenov approved these changes

Assignees

@sergey-semenov sergey-semenov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@steffenlarsen @romanovvlad @sergey-semenov @bader @alexbatashev