-
Notifications
You must be signed in to change notification settings - Fork 770
[SYCL] Use built-ins to retrieve kernel information #15070
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
This is a draft PR, the built-in path hasn't been tested since their support isn't ready yet. |
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 is more refactoring than I think is technically needed. That is perfectly ok though if you prefer this new interface.
The builtins were designed with the expectation that they would be used in the implementations of the KernelInfo
member functions. For example:
template <class KernelNameType> struct KernelInfo {
...
static constexpr const char *getName() {
#if !defined(__INTEL_SYCL_USE_INTEGRATION_HEADERS)
return __builtin_sycl_kernel_name(KernelIdentity<KernelNameType>());
#else
return "";
#endif
}
};
Note that the no-integration-header approach does not require or use explicit specializations of KernelInfo
(or KernelInfoData
).
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 my local testing I marked all the entry point functions as static functions since the attribute will throw an error if applied to non-static member function. Can we make that change in this PR as well? @tahonermann can explain it better but it is our understanding that it is an existing bug that these functions are not already marked as static.
I don't see any reason not to make that change, they probably should have been static from the start. Done. |
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.
The changes look ok to me, pending resolution of @aelovikov-intel 's comments
It was mistakenly removed in #15070
It was mistakenly removed in intel#15070
Using built-ins is going to be the preferred way to fetch kernel information, while integration headers are still going to be used for cases where built-ins are unavailable (i.e., different host compiler).
Additionally, switch to the new entry point attribute when using the built-ins.