[SYCL] sycl::kernel::get_kernel_bundle() may return a kernel_bundle with a null impl. #6598
Conversation
|
@zahiraam It looks like this change causes multiple LIT test failures, please address them. |
sycl/source/handler.cpp
Outdated
| @@ -77,11 +77,25 @@ handler::getOrInsertHandlerKernelBundle(bool Insert) const { | |||
| } | |||
|
|
|||
| // Sets kernel bundle to the provided one. | |||
| // If Kernel doesn't a kernel_bundle associated with it, the handler would | |||
| // still create a kernel_bundlxbe from a nullptr kernel_bundle_impl. This would | |||
There was a problem hiding this comment.
| // still create a kernel_bundlxbe from a nullptr kernel_bundle_impl. This would | |
| // still create a kernel_bundle from a nullptr kernel_bundle_impl. This would |
sycl/source/handler.cpp
Outdated
| @@ -77,11 +77,25 @@ handler::getOrInsertHandlerKernelBundle(bool Insert) const { | |||
| } | |||
|
|
|||
| // Sets kernel bundle to the provided one. | |||
| // If Kernel doesn't a kernel_bundle associated with it, the handler would | |||
| // still create a kernel_bundlxbe from a nullptr kernel_bundle_impl. This would | |||
| // have an invlaid impl and triggers an assertion in the handler. To remedy the | |||
There was a problem hiding this comment.
| // have an invlaid impl and triggers an assertion in the handler. To remedy the | |
| // have an invalid impl and trigger an assertion in the handler. To remedy the |
zahiraam
changed the title
|
@aelovikov-intel Is the LIT fail a know issue? I don't see that's related to the patch? |
|
I think it's related to this patch - you've added a new method that is visible outside libsycl.so. The failing test has instructions on how to update the CHECKs. |
This is the command that I am using to add the symbol, but I am getting an error: |
|
CUDA failure in Assert/assert_in_simultaneously_multiple_tus.cpp is known and the test has since been disabled. |
Thanks @steffenlarsen . @againull Can this be checked in? Thanks. |
There was a problem hiding this comment.
Functionally this change is good. I'd change some comments (see inline) but it might be just me.
@againull , WDYT?
Thanks @aelovikov-intel and @againull. Done. |
|
This PR seems to have some unrelated changes, for example those changes in "sycl/doc/design/OptionalDeviceFeatures.md". Is this due to a merge mistake? |
Apologies, @gmlueck. There was some problems in a rebase. We are fixing it now. |
…ith a null impl. This is an issue complaining about that return from getSyclObjImpl can be null. I talked to @steffenlarsen off line and although this is considered to be impossible (every constructor should create for itself and impl), it doesn't seem to be enforced anywhere in the code. We decided that would be a good compromise to make sure that impl is always non-null. Signed-off-by: Zahira Ammarguellat <[email protected]>
|
The PR is now clean. Thanks to @steffenlarsen who spent time looking at my ws and cleaning it up! |
Yes it is. Sorry about the confusion. |
| void handler::setHandlerKernelBundle(kernel Kernel) { | ||
| // Kernel may not have an associated kernel bundle if it is created from a | ||
| // program. As such, apply getSyclObjImpl directly on the kernel, i.e. not | ||
| // the other way around: getSyclObjImp(Kernel->get_kernel_bundle()). |
There was a problem hiding this comment.
I think this is not directly related to your change, but this comment makes me think there is a problem someplace. Why is there no kernel bundle for a kernel created from a program? The SYCL spec defines kernel::get_kernel_bundle() for all kernels, even those created from backend interop.
There was a problem hiding this comment.
Arguably there is, but since program is being removed I don't think it is worth the worry.
There was a problem hiding this comment.
OK, I was wondering if the issue about having no kernel bundle is limited to just the deprecated program case, or if it also included other cases like constructing a kernel from backend interop. If it is only the program case, I agree there is no need to fix it now. We should instead wait until program is removed.
There was a problem hiding this comment.
This problem should be unique to kernel bundles in kernels created from sycl::program. For sycl::make_kernel we either use a supplied kernel bundle (part of the L0 input type) or an empty "interop" kernel bundle.
The SYCL 2020 specification states that the instance of a moved SYCL object is invalid, so any operations after a move exhibit undefined behaviour. sampler.cpp moves a sampler before hashing the moved object. In that case, the hasher will extract a nullptr implementation, which with intel/llvm#6598 will fail an assertion. Since this is UB, failing the assertion is valid and the UB case should be removed. This commit removes the UB operation and removes the unnecessary opencl_icd requirement in sampler.cpp. Signed-off-by: Larsen, Steffen <[email protected]>
The SYCL 2020 specification states that the instance of a moved SYCL object is invalid, so any operations after a move exhibit undefined behaviour. sampler.cpp moves a sampler before hashing the moved object. In that case, the hasher will extract a nullptr implementation, which with intel/llvm#6598 will fail an assertion. Since this is UB, failing the assertion is valid and the UB case should be removed. This commit removes the UB operation and removes the unnecessary opencl_icd requirement in sampler.cpp. Signed-off-by: Larsen, Steffen <[email protected]>
The SYCL 2020 specification states that the instance of a moved SYCL object is invalid, so any operations after a move exhibit undefined behaviour. sampler.cpp moves a sampler before hashing the moved object. In that case, the hasher will extract a nullptr implementation, which with intel#6598 will fail an assertion. Since this is UB, failing the assertion is valid and the UB case should be removed. This commit removes the UB operation and removes the unnecessary opencl_icd requirement in sampler.cpp. Signed-off-by: Larsen, Steffen <[email protected]>
This is an issue complaining about that return from getSyclObjImpl can be null.
I talked to @steffenlarsen off line and although this is considered to be impossible (every constructor should create for itself and impl), it doesn't seem to be enforced anywhere in the code. We decided that would be a good compromise to make sure that impl is always non-null.