-
Notifications
You must be signed in to change notification settings - Fork 115
[oneDPL][async] sort_by_key_async prototype #1009
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
base: main
Are you sure you want to change the base?
Conversation
f50c0c3 to
c9a4a56
Compare
c9a4a56 to
2075e64
Compare
| auto | ||
| operator()(_ExecutionPolicy&& __exec, _Range&& __rng, _Merge __merge, _Compare __comp) const | ||
| operator()(_ExecutionPolicy&& __exec, _Range&& __rng, _Merge __merge, _Compare __comp, | ||
| const std::vector<sycl::event>& __dependency_events) 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.
Why do we hardcode it as a std::vector<sycl::event>? I thought it should still be Events&&... where one pf possible types could be std::vector<sycl::event> or even broader - any sequence of sycl::events. I don't feel that making it non-optional parameter is a good idea
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.
Yes, it s not ideal API, I agree.
Is it a draft, it is a "quick" solution for MKL team - to quick experiments that they need. Also the need to pass std::vectorsycl::event as the dependencies. Of course for productization we have to think more about 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.
So we ended up being able to use the synchronous sort_by_key in HPCG for GPU benchmark (from oneMKL team) but I am curious if you have any plans for when the non-blocking asynchronous apis which can take in a const std::vector<sycl::event> & like object would be targeted for? Do you have ideas about how to handle this with regards to your current Events&&... design ? It seems common in SYCL language in all the oneAPI libraries besides oneDPL to use a const std::vector<sycl::event> & for inputs of dependencies and return a single sycl::event to track outgoing dependencies. How do you plan to support such things in oneDPL so it is interoperable ?
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.
It is a good question.. Probably, async API for oneDPL will be revised before productization (currently it is the experimental API). As option, we might add an overload version of an async algo with const std::vector<sycl::event> & parameter. Also, we might implement a implicit conversion of a variadic pack Events&&... (convertible to sycl::event) to std::vector<sycl::event> and pass it to SYCL further.
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.
... I thought it should still be
Events&&...where one pf possible types could bestd::vector<sycl::event>
It is a good point. I agree with that, if we will not face with something issues connected with compile time processing such parameter pack.
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.
so in all other oneAPI libraries (besides oneDPL), we use const std::vector<sycl::event> & deps = {} which allows it to be optional and allows for passing in both a vector of events, or constructing vectors on the fly based on individual events passed in... The following list highlights approaches that are regularly used all over the place in other oneAPI libraries and user codes and we would hope would be supported by any oneDPL solution as well...
// std::vector<sycl::event> deps;
auto ev1 = func_async(..., deps);
auto ev2 = func2_async( ...); // implicitly say no dependencies (takes advantage of "={}" optional arg
auto ev3 = func3_async( ..., {ev1}); // automatically constructs to a const std::vector<sycl::event> & deps inside of length 1 with ev1 in it
auto ev4 = func4_async( ..., {ev1, ev2}); // automatically constructs to const std::vector<sycl::event> & deps inside of length 2 with ev1,ev2 in it
auto ev5 = func5_async( ..., {}); // explicitly say there are no dependencies
...
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.
@spencerpatty, thank you for the examples. I think is very useful and it is an additional input to find out the "ideal" oneDPL async 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.
Am I correct that with your current approach, these would all need to switch over to the follwoing ?
auto ev1 = func_async(..., deps);
auto ev2 = func2_async( ...); // implicitly say no dependencies
auto ev3 = func3_async( ..., ev1);
auto ev4 = func4_async( ..., ev1, ev2);
// auto ev5 = func5_async( ..., {}); // (not currently possible with EventList &&... type ) explicitly say there are no dependencies
[oneDPL][async] sort_by_key_async prototype
The dependencies are passed as std::vectorsycl::event.
In ideal id should be a kind of "view" - pointer and size... like std::span...