-
Notifications
You must be signed in to change notification settings - Fork 738
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
[SYCL][DOC] Propose "get_kernel_info" extension #14472
Conversation
Add a proposed extension to query a kernel's information descriptor without creating a kernel bundle.
Also improve the wording for constraints.
don't think we want to mandate an error check at runtime. | ||
In retrospect, I think this is the right behavior for the core SYCL spec also, | ||
and we should consider changing the specified behavior. | ||
Thoughts? |
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.
Which exceptions are these, specifically?
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.
For example:
Preconditions: [...] The device
dev
must be one of the devices contained byctxt
. The kernelKernelName
must be compatible with the devicedev
as defined byis_compatible
.
In the existing SYCL 2020 spec, these error conditions mandate an exception:
An
exception
with theerrc::invalid
error code if any of the devices indevs
is not one of devices contained by the contextctxt
or is not a descendent device of some device inctxt
.An
exception
with theerrc::invalid
error code if the kernel is not compatible with devicedev
(as defined byis_compatible()
)
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.
Actually enqueuing the kernel would still throw one of these exceptions though, 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.
This PR only adds query functions, so nothing in this PR would change the behavior when enqueuing a kernel.
My comment about "in retrospect", though, might change the behavior when enqueuing a kernel. I do feel that we went too far in the core SYCL spec about mandating exceptions for error conditions. This requires an implementation to check the function inputs and throw a specific exception. I'm not sure we want to mandate those error checks, especially for functions that are likely to be on an application's critical path. If we did not mandate the error checks, an implementation might just assume they are correct, and you get what you get if they are not. This might be a confusing error from a lower-level software layer, a crash, or even code that runs incorrectly.
Of course, an implementation could still perform these error checks. DPC++ could decide to do this by default if the cost isn't high. Or, DPC++ could provide a compiler option to enable them if they are expensive.
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.
Of course, an implementation could still perform these error checks. DPC++ could decide to do this by default if the cost isn't high. Or, DPC++ could provide a compiler option to enable them if they are expensive.
Are we allowed to go the other way?
There are already some cases (e.g., math) where a user can opt-in to disabling features required by the specification (e.g., certain NaN handling) in pursuit of performance. As long as DPC++ provides a mode that is conformant to the required errors, is it legal to also provide something like an -fsycl-no-exceptions
option (and maybe even enable it by default)?
If what I describe above is legal, I think I'd prefer it to adding more UB into the specification. Developers are already used to implementations providing options that improve performance, and specifying required exceptions guarantees that any conformant implementation will provide some configuration in which it's possible to get the safe behavior.
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 think it would be possible, but it seems contrary to the way C++ is specified. Most C++ library functions have a precondition that their arguments are correct. It's not common for C++ to mandate an error check.
However, this seems different from a _Constraint_, which is expected to result | ||
in a compile-time error. | ||
For now, I just listed it as a _Precondition_, so there is no formal | ||
requirement for an implementation to diagnose this error. |
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.
Seems like the specification currently uses precondition
for template argument requirements, so I think precondition is fine.
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.
Seems like the specification currently uses
precondition
for template argument requirements
In the current SYCL 2020 specification, you mean? Yes, this is a bug in the specification, and it's my fault. I wrote that part of the specification before I understood the difference between Preconditions and Constraints. Those should be Constraints. It's on my list of things to clean up.
In the calls to `get_kernel_info` allow the device to be a descendent device of the provided context.
@steffenlarsen have your concerns been resolved? I think @Pennycook's latest comments are related to the core SYCL spec, not this PR, so we can resolve them offline. |
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! Let's roll with it. 🪨
@intel/llvm-gatekeepers I think this is ready to merge. |
Add a proposed extension to query a kernel's information descriptor without creating a kernel bundle.