[SYCL] Fix for native_specialization_constant() #9085
Conversation
| @@ -112,6 +112,8 @@ class device_image_impl { | |||
| bool all_specialization_constant_native() const noexcept { | |||
| // Specialization constants are natively supported in JIT mode on backends, | |||
| // that are using SPIR-V as IR | |||
| if (!MBinImage) | |||
| return false; | |||
There was a problem hiding this comment.
Is this actually always the correct value to return here? Looking at the test example, shouldn't it depend on the binary image used for this? It seems like the actual issue is that we're losing track of this information after compile & link. Am I missing something here?
There was a problem hiding this comment.
The original issue is segmentation fault because of quering nullptr: MBinImage->getRawData() - I thought MBinImage was meant to be nullptr, and we just need to check it. Do you think it shouldn't?
BTW yes, native_specialization_constant() depends on the binary image.
There was a problem hiding this comment.
MBinImage might have been meant to be nullptr in this "input -> compile -> link" case in terms of its other uses in the code, but then checking it like that doesn't seem to be sufficient for native_specialization_constant(). I would expect native_specialization_constant() to return true in the added test case if it was true for the original input kernel_bundle.
There was a problem hiding this comment.
So, I tried to preserve BinImage during the linking so there is no nullptr. Looks like it works, but need to get CI results 16bfd69
There was a problem hiding this comment.
Okay, CI passed. I'm wondering if we should do something in case MBinImage is nullptr - should there be an assert/exception?
There was a problem hiding this comment.
If I understand correctly, an assertion won't do because we can still have a nullptr MBinImage if the kernel bundle is created via interop, so that case should be handled as well.
There was a problem hiding this comment.
Hmm, I made some experiments, and I'm not sure if such case is even possible now, as the only one way to get the interop bundle - use make_kernel(), but is it possible to use spec constant with such kernel? I made a plug for now.
KornevNikita
changed the title
|
TEST 'SYCL :: Regression/same_unnamed_kernels.cpp' is unrelated |
|
@sergey-semenov take one more look, please. |
Co-authored-by: Steffen Larsen <[email protected]>
|
Failed Tests (1): |
If MBinImage is nullptr, it leads to SIGSEGV.
This patch fixes ProgramManager::link() function, so MBinImage will not be nullptr.
Also changes link() signature to accept a single device image instead of vector,
because we use it to pass the vector of one image, so it makes no sense.