-
Notifications
You must be signed in to change notification settings - Fork 804
Handler-less kernel submit API #19294
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: sycl
Are you sure you want to change the base?
Conversation
sycl/include/sycl/queue.hpp
Outdated
const detail::code_location &CodeLoc, | ||
bool IsTopCodeLoc) const; | ||
|
||
event submit_with_event_impl( |
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.
What about eventless? It is not done yet, right?
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, I think it would be similar, so I've skipped it for now.
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.
What is the disadvantage of returning optional<event>
and having somewhere (probably, in SubmissionInfo
, as this is mode of submission) a flag, pointing out is it event or eventless mode? I think about bunch of functions that pass arguments by chain and about duplicating them (for event and for eventless) and this is not looks good. What do you think?
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 am not sure if returning std::optional<event>
is a good idea because of ABI concerns. It might not have a stable ABI across compiler versions or even different standard libraries (libstdc++ vs libc++).
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 am not sure if returning
std::optional<event>
is a good idea because of ABI concerns. It might not have a stable ABI across compiler versions or even different standard libraries (libstdc++ vs libc++).
Yes, good point.
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.
Probably, sycl::detail::optional
might be considered.
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.
sycl::detail::optional might work, 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.
But still we need to care about the stable layout of the sycl::detail::optional
. I am not sure that we are doing it today.
I think having two versions (that return sycl::event
and return void
) might be a good alternative.
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.
We don't generally do that and we rely on backward compatibility guarantees of the C++ library we use (GNU libstdc++ on Linux/MSVC on Windows). The only exception is pre-C++11 ABI of GNU libstdc++ that pyTorch used to use (see https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html). I don't see std::optional
listed on that page, so we should be safe to use it.
Another possible caveat is if some STL's implementation of it isn't is_sycl_device_copyable
. I think that might have been a reason why we added sycl::detail::optional
(or maybe it was simply added when we used C++14, `std::optional' is C++17 and above).
Anyway, unless you have a known case when it doesn't work, the current approach in the rest of the project is to use std::optional
, AFAIK.
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 PR, I would like to see at least one public interface implementation that utilizes this approach, just to ensure it works.
In the latest update, there are two public interfaces: The enqueue functions extension, and queue.parallel_for. Both are enabled only if __DPCPP_ENABLE_UNFINISHED_NO_CGH_SUBMIT is defined. |
expose the new APIs as public under a new define
…eric part. Added "kernel" to the direct submission function names.
No description provided.