-
Notifications
You must be signed in to change notification settings - Fork 796
[SYCL] Extend the handler-less path to API functions with event dependencies #20461
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
Conversation
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.
LGTM, but address the suggestion I left with #pragma once
Co-authored-by: Sergei Vinogradov <[email protected]>
|
@mmichel11 @intel/sycl-graphs-reviewers @uditagarwal97 @sergey-semenov @aelovikov-intel Could you please take a look? |
| KernelType, sycl::nd_item<1>>::value)) { | ||
| detail::submit_kernel_direct_parallel_for(q, nd_range<1>(r, size), | ||
| std::forward<KernelType>(k)); | ||
| std::forward<KernelType>(k), {}); |
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 wonder if we should set a default value for DepEvents in submit_kernel_direct_parallel_for and submit_kernel_direct_single_task, so that we can avoid passing {} everywhere.
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.
That doesn't matter. Once handler-less path "implementation" finishes, next step would be to clean up the codebase and that would include dropping it completely and re-writing handler path to delegate to the public handler-less APIs to avoid extra level of templates.
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.
Graph changes LGTM
| /// be nullptr if no associated graph. | ||
| /// \param CommandGroupType Type of command group. | ||
| template <bool LockQueue = true> | ||
| void registerEventDependency( |
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.
Instead of creating a new header, is it possible to put this function in some existing header, like event_impl.hpp?
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.
Yeah, this made sense to me too, but unfortunately there is a circular dependency between the event_impl.hpp and queue_impl.hpp..
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.
LGTM overall. I don't see any test associated with this change, does any existing test(s) cover them?
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.
LGTM, no blocking comments, just some non-functional nitpicks.
|
@intel/llvm-gatekeepers please consider merging |
I've added a test for the API function, which was not covered, but the rest of the functions affected by this PR should be covered by existing tests. |
The kernel submission APIs with event dependencies are switched to use the handler-less kernel submission path.