-
Notifications
You must be signed in to change notification settings - Fork 749
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
base: sycl
Are you sure you want to change the base?
Conversation
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.
3b385e3
to
f447b82
Compare
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.
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]+]]} |
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.
Minor documentation nit: The PR description hints at a named MD !sycl_registered_kernels
being emitted, whereas the implementation actually uses a module flag.
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.
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.
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.
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?
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.
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.
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.
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?
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.
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!
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.
LGT-not-super-familiar-with-the-frontend-me, thanks!
@intel/dpcpp-cfe-reviewers, could you please review? |
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.
@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.
This change adds support for a new attribute
__sycl_detail::__registered_kernels__
, which appears at translationunit scope. The parameter for this attribute is a list of pairs like:
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:
where the first element in the pair of strings, is the first element
of the pair in
__registered_kernels__
and the second element is themangled named of the wrapper corresponding to the free function.