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] Add support for __registered_kernels__ #16485

Open
wants to merge 9 commits into
base: sycl
Choose a base branch
from

Conversation

premanandrao
Copy link
Contributor

@premanandrao premanandrao commented Dec 27, 2024

This change adds support for a new attribute
__sycl_detail::__registered_kernels__, which appears at translation
unit scope. The parameter for this attribute is a list of pairs like:

[[__sycl_detail__::__registered_kernels__(
  {"foo", foo},
  {"(void(*)(int, int*))iota", (void(*)(int, int*))iota},
  {"kernel<float>", kernel<float>}
)]];

The first element in each pair is a string, and the second element is a
constant expressiton for a pointer to a SYCL free function kernel.

The change creates the kernel's wrapper function and generates
module-level metadata of the form:

!sycl_registered_kernels = !{!0, !1}
!0 = !{!"foo", !"mangled-name-of-wrapper-for-foo"}
!1 = !{!"kernel<float>", !"mangled-name-of-wrapper-for-kernel<float>"}

where the first element in the pair of strings, is the first element
of the pair in __registered_kernels__ and the second element is the
mangled named of the wrapper corresponding to the free function.

@premanandrao premanandrao requested a review from a team as a code owner December 27, 2024 05:59
@premanandrao premanandrao changed the title Remote reg kern [SYCL] Add support for __registered_kernels__ Dec 27, 2024
This change adds support for a new attribute
__sycl_detail::__registered_kernels__, which appears at translation
unit scope.  The parameter for this attribute is a list of pairs like:

``
[[__sycl_detail__::__registered_kernels__(
  {"foo", foo},
  {"(void(*)(int, int*))iota", (void(*)(int, int*))iota},
  {"kernel<float>", kernel<float>}
)]];
``

The first element in each pair is a string, and the second element is a
constant expressiton for a pointer to a SYCL free function kernel.

The change creates the kernel's wrapper function and generates
module-level metadata of the form:

``
!sycl_registered_kernels = !{!0, !1}
!0 = !{!"foo", !"mangled-name-of-wrapper-for-foo"}
!1 = !{!"kernel<float>", !"mangled-name-of-wrapper-for-kernel<float>"}
``

wherre the first element in the pair of strings, is the first element
of the pair in __registered_kernels__ and the second element is the
mangled named of the wrapper corresponding to the free function.
Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a real review, but I played with this locally and it LGTM for the SYCL-RTC use-case, thanks!


// Check for the presence of sycl-device module flag in device
// compilations and its absence in host compilations.
// CHECK: !{{[0-9]+}} = !{i32 5, !"sycl_registered_kernels", ![[LIST:[0-9]+]]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor documentation nit: The PR description hints at a named MD !sycl_registered_kernels being emitted, whereas the implementation actually uses a module flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. It was a thinko on my part.
Please let me know if I should change it - I am not really sure which has advantages over the other.

Copy link
Contributor

@jopperm jopperm Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know if I should change it - I am not really sure which has advantages over the other.

Will you be tackling the transformation of this metadata to a property set as well? Maybe this can inform the decision. Anecdotally, the ComputeModuleRuntimeInfo utility (in SYCLLowerIR) doesn't look at module flags right now, only named MD.

@gmlueck Do you have a strong(er) opinion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you be tackling the transformation of this metadata to a property set as well?

No, that won't be me.

I will wait for Greg to confirm, but it looks like it should be a named metadata then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, module flags are a special kind of named MD which has clear semantics for how the named MD is merged together when IR modules are merged. I think the merging semantics are not important for sycl_registered_kernels, though, because the kernel compiler will never merge together multiple IR modules. We will always compile the IR straight to SPIR-V (or PTX, etc.)

Therefore, I think using named MD would be OK. On the other hand, I can't think of any reason why it would be problematic to use module flags either. If it's easier to consume named MD from sycl-post-link, I think we could go with named MD, but I don't have a strong preference.

@cperkinsintel I think you are coordinating work for the kernel compiler. Who will be responsible for converting sycl_registered_kernels into a property set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, I think using named MD would be OK.

I changed it to use named MD now. It really was a mistake on my part initially to use module flags, but then I got curious as to how it was different. Your explanation that it is a special case of named MD makes perfect sense. Thanks!

Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGT-not-super-familiar-with-the-frontend-me, thanks!

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCLDeclAttr.cpp Outdated Show resolved Hide resolved
@premanandrao
Copy link
Contributor Author

@intel/dpcpp-cfe-reviewers, could you please review?

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.

@Fznamznon can you take a look at this? I was unable to today and will be out for part of the day tomorrow. I don't want to hold this up.

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