[SYCL] Update the kernel function pointer on function copy/move #20389
Conversation
slawekptak temporarily deployed to WindowsCILock 
GitHub Actions Inactive slawekptak temporarily deployed to WindowsCILock GitHub Actions Inactive slawekptak temporarily deployed to WindowsCILock GitHub Actions Inactive The kernel function pointer, used to extract the kernel arguments, needs to point to the user provided function, or a function copy, if the copy/move is needed.
There was a problem hiding this comment.
@mmichel11 We should also add your regression test, can do as a follow-up PR.
| // No need to copy/move the kernel function, so we set | ||
| // the function pointer to the original function | ||
| KData.setKernelFunc(HostKernel.getPtr()); |
There was a problem hiding this comment.
Why not just update decltype(KData) to use HostKernelBase & instead of a raw pointer?
There was a problem hiding this comment.
There is void handler::setKernelInfo left for compatibility, which sets the pointer. Wouldn't this be an issue?
There was a problem hiding this comment.
I assumed that KData is of type recently added by @vinser52 that hasn't made its way into any official release and we can freely modify it. Isn't that the case?
There was a problem hiding this comment.
That's true, but the function pointer was moved into it, and now both new and old code uses it to store the pointer. Also, KernelData only needs the pointer, so why do you think storing HostKernelBase is better?
There was a problem hiding this comment.
@aelovikov-intel I believe @slawekptak is talking about ABI entry point (handler::setKernelInfo). My idea was that when ABI window is opened we can cleanup this part. Today in a hadler path, the user functor is set two times inside StoreLambda functions:
void StoreLambda(...) { // first time MHostKernel.reset(new detail::HostKernel<KernelType, LambdaArgType, Dims>( std::forward<KernelTypeUniversalRef>(KernelFunc))); // second time setDeviceKernelInfo<KernelName>((void *)MHostKernel->getPtr()); }It should be fixed
There was a problem hiding this comment.
Also, KernelData only needs the pointer, so why do you think storing HostKernelBase is better?
Because function pointer can go stale, HostKernelBase can't.
There was a problem hiding this comment.
In the case of handler path, we are storing the HostKernelBase anyway (as I wrote above). So I agree with @aelovikov-intel.
5 participants
The kernel function pointer, used to extract the kernel arguments, needs to point to the user provided function, or a function copy, if the copy/move is needed.