llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[SYCL] Initial changes for C++11 ABI=0 support

Open bso-intel opened this issue 2 years ago • 14 comments

This PR attempts to support the usage case where the user sets _GLIBCXX_USE_CXX11_ABI=0 to use pre-C++11 ABI. In fact, this change addresses a specific issue with using different versions of the libstdc++ library (https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html has more details on this issue).

One of the major changes I made in this PR involves calling get_info<>() API, which can return stdA::string as the requested information. Due to the ABI incompatibility issues, this API internally splits into 3 cases depending on the template parameter <T>.

  1. <T>s that return a std::string.
  2. <T>s that return a std::vector<std::string>
  3. <T>s that return something other than 1 or 2 above.

The case 1 and 2 should return detail::string and std::vector<detail::string> instead and reconstruct std::strings. This is required because ABIs can be different between the header and CPP files. All these 3 cases are implemented using get_info_impl<T>. Then, I changed the macro definition of __SYCL_PARAM_TRAITS_SPEC to return different types depending on the <T> return_types. This way, we can only change the boundary between the header file and the entry point of the libsycl.

bso-intel avatar Dec 15 '23 18:12 bso-intel

@intel/llvm-reviewers-runtime @gmlueck @bader Here is the initial PR that I presented in the DPCPP technical forum. The intention of this PR is to get some early feedback from you. Please do not worry about the test failures shown here since I did not change the test sources yet.

By the way, it seems I have to put some guard #ifdef __INTEL_PREVIEW_BREAKING_CHANGES because this PR breaks the ABI and we are not allowed to merge ABI-breaking changes without this guard.

bso-intel avatar Dec 17 '23 18:12 bso-intel

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

github-actions[bot] avatar Jan 04 '24 20:01 github-actions[bot]

This PR attempts to support the usage case where the user sets _GLIBCXX_USE_CXX11_ABI=0 to use pre-C++11 ABI. In fact, this PR makes SCYL C++11-ABI neutral so it does not matter whether the user wants pre-C++11_ABI=0 or =1.

SCYL -> SYCL

I'm not sure that the term "neutral C++11-ABI" is so well known that everyone understands it the same way. I think it's worth mentioning that this change addresses a specific issue with using different versions of the libstdc++ library (https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html has more details on this issue).

bader avatar Jan 10 '24 21:01 bader

Moving the discussion on #12393 to here:

We (the kernel fusion team) could re-use a std::string replacement class in our effort to remove STL classes from the interface between the JIT compiler driver in the runtime and our fusion library. However, the marshalling approach alone does not work well for us because one of our uses of std::string is as a struct member that is passed in both directions across the interface. Hence, we'd like to have a type with clear owning semantics, and corresponding copy and move constructors/assignments.

@bso-intel Do you think we can extend your proposal to cover our use-case as well?

jopperm avatar Jan 23 '24 00:01 jopperm

Moving the discussion on #12393 to here:

We (the kernel fusion team) could re-use a std::string replacement class in our effort to remove STL classes from the interface between the JIT compiler driver in the runtime and our fusion library. However, the marshalling approach alone does not work well for us because one of our uses of std::string is as a struct member that is passed in both directions across the interface. Hence, we'd like to have a type with clear owning semantics, and corresponding copy and move constructors/assignments.

@bso-intel Do you think we can extend your proposal to cover our use-case as well?

Sure. I recommend inheriting from sycl::detail::string class so it can be customized for your usage cases. Please note that sycl::detail::string is intended to solve the ABI incompatibility issue. So, there is some limitation on how you add/return std::string. By inheriting from sycl::detail::string, you don't need to worry about this incompatibility issues.

bso-intel avatar Jan 23 '24 01:01 bso-intel

@jopperm I refactored this PR to have two separate classes sycl::detail::string and sycl::detail::string_view.

bso-intel avatar Jan 24 '24 00:01 bso-intel

I would like to make some general comments here to the reviewers. I appreciate your feedback, but I got the impression that the reviewers think sycl::detail::string is just a replacement of std::string. It is not. It is rather a cosmetic wrapper of char *. As I mentioned in the string.hpp, this is just a simple wrapper of native pointer char *. detal::string is not meant to be used as a replacement of std::string. It is used to send/receive "char *" when the control crosses two different ABIs. std::string has different ABIs between pre-C++11 ABI and post-C++11 ABI. So, our customers who need to use pre-C++11 ABI cannot use SYCL which only supported post-C++11 ABI. The lifetime of this class object is very short, just one function call. An object is created to extract "char *" from one ABI side and send it to the other ABI side, and reconstruct std::string. There can't be any intervening instructions between marshalling and unmarshalling operations. Further, there is a significant limitation on passing and returning std::string in this dual ABI scenario. I will try to address your feedback as much as I can, but please understand not everything is possible since detail::string is not simply a replacement of std::string.

bso-intel avatar Jan 24 '24 18:01 bso-intel

Thanks, @victor-eds and @aelovikov-intel I think I addressed all your feedback. Some of them cannot be satisfied due to the restriction of the dual ABI usage case. If I missed anything, please let me know.

bso-intel avatar Jan 25 '24 04:01 bso-intel

@aelovikov-intel @victor-eds Thank you for your additional feedback. I think I addressed all your feedback. Please let me know if I missed anything.

bso-intel avatar Jan 25 '24 23:01 bso-intel

@jopperm I think I addressed all your feedback.

bso-intel avatar Jan 26 '24 03:01 bso-intel

Just curious about the compelling use case... Compiling SYCL for 20 year old systems?

keryell avatar Jan 27 '24 01:01 keryell

@aelovikov-intel changes would be good. string needs the special member functions being overriden, but not needed for string_view

@victor-eds , could you elaborate on what changes you are requesting?

bso-intel avatar Feb 07 '24 16:02 bso-intel

@victor-eds @aelovikov-intel I think I address all your feedback.

bso-intel avatar Feb 08 '24 04:02 bso-intel

@sergey-semenov @aelovikov-intel I think I addressed all your feedback. Is there anything I have to address further?

bso-intel avatar Feb 16 '24 22:02 bso-intel

Failures are not related to my PR.

Failed Tests (7):
  SYCL :: Assert/assert_in_kernels_win.cpp
  SYCL :: Assert/assert_in_multiple_tus_one_ndebug_win.cpp
  SYCL :: Assert/assert_in_multiple_tus_win.cpp
  SYCL :: Assert/assert_in_one_kernel_win.cpp
  SYCL :: Assert/assert_in_simultaneous_kernels_win.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp

bso-intel avatar Feb 23 '24 03:02 bso-intel

@aelovikov-intel @sergey-semenov Are there anything else I have to address?

bso-intel avatar Feb 23 '24 17:02 bso-intel

@aelovikov-intel @sergey-semenov I think I addressed all your feedback. Is there anything I have to further address?

bso-intel avatar Feb 24 '24 01:02 bso-intel

@sergey-semenov @aelovikov-intel friendly ping.

bso-intel avatar Feb 27 '24 05:02 bso-intel