-
Notifications
You must be signed in to change notification settings - Fork 113
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
Lazy sycl::queue
creation in oneDPL hetero policies
#1422
Lazy sycl::queue
creation in oneDPL hetero policies
#1422
Conversation
sycl::queue
creation in oneDPL hetero policies
8613cc4
to
2ebd0ab
Compare
dfb26ce
to
b46bc59
Compare
} | ||
|
||
auto | ||
get_sycl_queue_container() const |
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.
Do we need this function as part of public API?
@@ -78,32 +126,50 @@ class device_policy | |||
} | |||
|
|||
private: | |||
sycl::queue q; | |||
mutable sycl_queue_container_ptr<TSyclQueueFactory> q_container; |
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.
Just an idea:
Consider changing sycl_queue_container_ptr::get_queue()
to be const
and sycl_queue_container_ptr::__queue
to be mutable.
For me, it can be unclear why q_container
is mutable from the device_policy
implementation perspective.
//We can create device_policy object: | ||
// 1. from sycl::queue | ||
// 2. from sycl::device_selector (implicitly through sycl::queue) | ||
// 3. from sycl::device | ||
// 4. from other device_policy encapsulating the same queue type | ||
template <typename KernelName = DefaultKernelName> | ||
template <typename KernelName = DefaultKernelName, class TSyclQueueFactory = sycl_queue_factory_device> |
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.
As far as I can see in oneDPL specification - device_policy
class is a public API and hence its template parameters are also documented. So do we think that it is necessary to have TSyclQueueFactory
template parameter as part of public API?
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.
In this valiant - probably yes.
b46bc59
to
3f19cba
Compare
48831a0
to
f8a863c
Compare
f8a863c
to
8726406
Compare
a69806f
to
733892d
Compare
e6b5b9d
to
c724558
Compare
c724558
to
2cdad2e
Compare
d102f95
to
03b8f22
Compare
03b8f22
to
5ed3583
Compare
8ad5361
to
dc5bcf7
Compare
dc5bcf7
to
9de5fc0
Compare
9de5fc0
to
68a90c1
Compare
68a90c1
to
ce03c8d
Compare
…DefaultSYCLQueueFactory to sycl_queue_factory_device
…DefaultSYCLQueueFactoryFPGA to sycl_queue_factory_fpga
…ormat of the make_device_policy functions to avoid create sycl::queue through device_policy::operator sycl::queue()
ce03c8d
to
12d1a93
Compare
After merging #1652, this patch is outdated. |
Attention: ABI breaking change.
Yet another approach to lazy
sycl::queue
creation in oneDPL hetero policies:The main implementation detail is
std::once_flag
and::std::call_once
usage packed into separate containerThis container shares between hetero policies instances:
From my point of view this approach is correct because it's simple repeat something like
sycl::queue
implementation (pimpl-implementation) and we don't introduce any new synchronization errors here.