[oneDPL] Added "in order" queue support #1260
Conversation
|
|
||
| public: | ||
| __event(sycl::event __e): __m_event(__e) {} | ||
| __event() {} |
There was a problem hiding this comment.
__event() = default;
There was a problem hiding this comment.
What's a advantages of that in given case?
There was a problem hiding this comment.
In common case =default doesn't change triviality state of class.
See https://ru.stackoverflow.com/questions/495342/%D0%A7%D0%B5%D0%BC-%D0%BF%D1%83%D1%81%D1%82%D0%BE%D0%B9-%D0%BA%D0%BE%D0%BD%D1%81%D1%82%D1%80%D1%83%D0%BA%D1%82%D0%BE%D1%80-%D0%BF%D0%BE-%D1%83%D0%BC%D0%BE%D0%BB%D1%87%D0%B0%D0%BD%D0%B8%D1%8E-%D0%BE%D1%82%D0%BB%D0%B8%D1%87%D0%B0%D0%B5%D1%82%D1%81%D1%8F-%D0%BE%D1%82-default for details.
There was a problem hiding this comment.
"doesn't change triviality state of class." - yes...
But, do we really need it? or it can be useful in the Future? I'm just asking...
BTW, the same details in English: https://stackoverflow.com/questions/20828907/the-new-syntax-default-in-c11
There was a problem hiding this comment.
This general idea look like as applied in all cases.
MikeDvorskiy
force-pushed
the
dev/mdvorski/in_order_queue_support
branch
2 times, most recently
from
d58f8d6 to
9099000
Compare
| //#if !ONEDPL_ALLOW_DEFERRED_WAITING | ||
| //__my_event.wait_and_throw(); | ||
| //#endif | ||
| __my_event.wait(); |
There was a problem hiding this comment.
Is this intentional? If so, what has changed here and why keep the old commented code?
There was a problem hiding this comment.
yes... (I forgot remove the old code)
| __m_event.wait_and_throw(); | ||
| } | ||
|
|
||
| bool has_event() const { return !__in_order; } |
There was a problem hiding this comment.
Is this a problem if the default constructor was called to create this __event? __in_order is false, but only a default constructed event exists.
I suppose in the way it is used currently, this isnt a problem. However, the naming of this function may be misleading. Maybe we would just be better off to assert(!__in_order); directly in the conversion function.
There was a problem hiding this comment.
Actually, I've redesigned that code a little bit.
| #if _ONEDPL_COMPILE_KERNEL | ||
| , typename _Kernel | ||
| #endif | ||
| > | ||
| sycl::event | ||
| auto |
There was a problem hiding this comment.
I think it may be better for code maintainability to use __dpl_sycl::__event as the return type rather than auto. (same for other similar functions in thie PR)
For the declarations, I don't mind the use of auto, but for returns of large functions, this seems like unnecessary obfuscation when the type is known and has a simple name.
| } | ||
|
|
||
| bool has_event() const { return !__in_order; } | ||
| operator sycl::event() const |
There was a problem hiding this comment.
Would it be safer to make this an explicit function call like get_sycl_event() rather than a conversion function?
I'm worried we might get ourselves into a bit of trouble with a conversion function since this is not always interchangeable with a sycl::event (as evidenced by the assert).
Is this only used in the __future implementation within this PR currently?
MikeDvorskiy
force-pushed
the
dev/mdvorski/in_order_queue_support
branch
3 times, most recently
from
ac4e89c to
f1b733d
Compare
…encies as __dpl_sycl::__event or sycl::event or std:vector<sycl::event>
MikeDvorskiy
force-pushed
the
dev/mdvorski/in_order_queue_support
branch
from
f1b733d to
d39c402
Compare
| auto | ||
| __submit(sycl::queue __queue, _Body __body) | ||
| { | ||
| return __submit_impl(__queue, __body, true, NULL, NULL); |
There was a problem hiding this comment.
Better to use nullptr keyword here and below.
[oneDPL] Added "in order" queue support.
We introduce
__dpl_sycl::__eventand__dpl_sycl::sumbit(..)wrappers.out_of_orderqueue the mentioned wrappers work as sycl::event and sycl::queue::submit, set sycl dependencies and return sycl event.in_orderqueue__dpl_sycl::sumbitdoesn't set sycl dependencies and dosn't return sycl event. To provide a sync oneDPL call__dpl_sycl::__eventcallssycl::queue::wait.