Skip to content
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] Use built-ins to retrieve kernel information #15070

Merged
merged 23 commits into from
Sep 10, 2024

Conversation

sergey-semenov
Copy link
Contributor

@sergey-semenov sergey-semenov commented Aug 14, 2024

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.

@sergey-semenov
Copy link
Contributor Author

This is a draft PR, the built-in path hasn't been tested since their support isn't ready yet.

sycl/include/sycl/detail/kernel_desc.hpp Show resolved Hide resolved
sycl/include/sycl/detail/kernel_desc.hpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Show resolved Hide resolved
Copy link
Contributor

@tahonermann tahonermann left a 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).

sycl/include/sycl/detail/kernel_desc.hpp Show resolved Hide resolved
sycl/include/sycl/detail/kernel_desc.hpp Outdated Show resolved Hide resolved
sycl/include/sycl/detail/kernel_desc.hpp Outdated Show resolved Hide resolved
sycl/include/sycl/detail/kernel_desc.hpp Outdated Show resolved Hide resolved
@sergey-semenov sergey-semenov marked this pull request as ready for review August 28, 2024 12:53
@sergey-semenov sergey-semenov requested a review from a team as a code owner August 28, 2024 12:53
Copy link
Contributor

@elizabethandrews elizabethandrews left a 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.

@sergey-semenov
Copy link
Contributor Author

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.

Copy link
Contributor

@elizabethandrews elizabethandrews left a 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

@sergey-semenov sergey-semenov merged commit c87f6b2 into intel:sycl Sep 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants