[SYCL][Fusion][NoSTL] Use custom string-like class to represent kernel name

[jopperm]

Implement a custom string class (backed by a DynArray<char>, hence owning its memory) to represent kernel names on the fusion interface.

This PR is part of a series of changes to remove uses of STL classes in the kernel fusion interface to prevent ABI issues in the future.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jopperm]

@intel/llvm-gatekeepers This is also ready to land, thanks.

[aelovikov-intel]

I'd like the folks I've added to have a chance to look into this before merging. It might be that we'd want a more broad/uniform solution between fusion and the rest of sycl.

[bader]

Totally agree with @aelovikov-intel.

This PR and #12193 from @bso-intel are trying to solve the same problem. There might be differences in requirements for the string replacement from Fusion and SYCL runtime, but we should avoid duplicating solutions.

[jopperm]

Ok, I'll investigate if we can use the class from #12193!

[jopperm]

@aelovikov-intel @bader After closer inspection, I think the use-case for our DynString class is a bit different than what sycl::detail::string is designed to achieve. For us, the std::string replacement needs to be a member of the SYCLKernelInfo struct, which is used to represent both the input kernels (created by the SYCL side) and the fused result (created/cached by the fusion library side) on the fusion interface. Hence, it's not trivial to follow a marshalling/unmarshalling approach without duplicating the kernel info struct or otherwise restructuring the interface. Considering that DynString is just around a dozen lines of code, do you think that the partially overlapping concerns could be acceptable in this situation?

[aelovikov-intel]

@aelovikov-intel @bader After closer inspection, I think the use-case for our DynString class is a bit different than what sycl::detail::string is designed to achieve. For us, the std::string replacement needs to be a member of the SYCLKernelInfo struct, which is used to represent both the input kernels (created by the SYCL side) and the fused result (created/cached by the fusion library side) on the fusion interface. Hence, it's not trivial to follow a marshalling/unmarshalling approach without duplicating the kernel info struct or otherwise restructuring the interface. Considering that DynString is just around a dozen lines of code, do you think that the partially overlapping concerns could be acceptable in this situation?

If sycl::detail::string was split into separate owning/view classes, would that enable you to use one of them? I'm not asking you to use what's suggested in #12193 arlready as-is, but rather influence it so that what we provide there is usable here.

[jopperm]

If sycl::detail::string was split into separate owning/view classes, would that enable you to use one of them? I'm not asking you to use what's suggested in #12193 arlready as-is, but rather influence it so that what we provide there is usable here.

Ah ok, yes, that should be possible. I see @victor-eds has already re-started the owning/view class discussion. I'll add that we'd want to have copy and move facilities for the owning variant to make it more convenient to use it a class member.

[sommerlukas]

@aelovikov-intel @bader Would you be OK with merging this PR as a temporary solution? We have a series of PRs building on top of each other and waiting for the design in #12193 to be settled would block our work.

I would propose that we merge this PR, our teams gets involved in the design discussion on #12193 (as @victor-eds and @jopperm already have) and once #12193 settled on a suitable design, we will follow up with a PR to use sycl::detail::string instead of our temporary solution.

WDYT?

[aelovikov-intel]

I'm ok with that... @bader ?

[bader]

Okay. @sommerlukas, @jopperm, I rely on you to ensure that this is a temporary solution. We don't want to end up having multiple implementations of "no-stl" strings across the project.

[jopperm]

Thanks all for the approval. I've filed an internal ticket to drop DynString once the other string replacement class becomes available.

@intel/llvm-gatekeepers Please merge, thanks!