-
Notifications
You must be signed in to change notification settings - Fork 795
[NFC][SYCL] Use CompileTimeKernelInfo instead of individual properties
#19968
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
[NFC][SYCL] Use CompileTimeKernelInfo instead of individual properties
#19968
Conversation
| // No-compile time info for the kernel (i.e., kernel_bundle/interop/etc.), | ||
| // be conservative: | ||
| if (NumParams == 0) | ||
| return true; |
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.
Sorry, I don't understand why are we returning true if there's no parameters...
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.
Basically, we had the following before
template <typename KernelNameType> constexpr bool hasSpecialCaptures() {
bool FoundSpecialCapture = false;
for (unsigned I = 0; I < getKernelNumParams<KernelNameType>(); ++I) {
auto ParamDesc = getKernelParamDesc<KernelNameType>(I);
bool IsSpecialCapture =
(ParamDesc.kind != kernel_param_kind_t::kind_std_layout &&
ParamDesc.kind != kernel_param_kind_t::kind_pointer);
FoundSpecialCapture |= IsSpecialCapture;
}
return FoundSpecialCapture;
}
Which returns false for hasSpecialCaptures when there are no parameters.
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 corresponds to the bool HasSpecialCaptures = true; on the left (which we chose in #19117 (comment)).
NumParams isn't really 0 unless we leave that data "uninitialized", like in the interop kernel code path where we only know its Name.
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 see.
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.
LGTM
Factored out from #19929.