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

bso-intel · 2023-12-15T18:45:44Z

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>.

<T>s that return a std::string.
<T>s that return a std::vector<std::string>
<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.

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. Signed-off-by: Byoungro So <[email protected]>

Signed-off-by: Byoungro So <[email protected]>

bso-intel · 2023-12-17T18:27:19Z

@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.

sycl/include/sycl/device.hpp

sycl/include/sycl/string.hpp

Signed-off-by: Byoungro So <[email protected]>

github-actions · 2024-01-04T20:21:28Z

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

Signed-off-by: Byoungro So <[email protected]>

sycl/source/device.cpp

sycl/source/platform.cpp

sycl/include/sycl/device.hpp

sycl/include/sycl/exception.hpp

aelovikov-intel · 2024-02-28T18:08:48Z

sycl/include/sycl/handler.hpp

@@ -3307,7 +3322,7 @@ class __SYCL_EXPORT handler {
  std::vector<detail::ArgDesc> MAssociatedAccesors;
  /// Struct that encodes global size, local size, ...
  detail::NDRDescT MNDRDesc;
-  std::string MKernelName;
+  detail::string MKernelName;


Every use of this seems to expect an std::string. Do we really need to change this data member, or maybe just a few places where it's getting assigned to?

Yes, this private data member should cross the ABI boundary.
So I had to convert it to detail::string to overcome the ABI incompatibility issue.

Oh, that brings another question. How are you testing that there are no std::strings crossing the boundary?

Also, have you tried changing callees accepting MKernelName instead of modifying the callers?

I can't check everywhere at this moment.
That's why this PR is only the initial changes as the title of PR says.
We will fix as we encounter from the customer's usage case.

Also, have you tried changing callees accepting MKernelName instead of modifying the callers?

I am not sure if I understand this question correctly, so please correct me if I misunderstood you.
MKernelName is defined in the headerfile, and the callees (cpp files) need this data member.
So, they accept MKernelName, but I can't hand over std::string since it is crossing the ABI boundary.
So, I changed MKernelName to detail::string and then the callee re-construct std::string in the CPP file.

Do they really need std::string? Can they keep using detail::string or detail::string_view?

Well.. That's called code pollution.
We (including @sergey-semenov) do not want to propagate detail::string deep on the CPP side.
detail::string and detail::string_view should be limited to be used when crossing the ABI boundary. They are not meant to replace std::string and std::string_view.

I only partially agree with that. I think many of these interfaces currently accept const std::string & and they have no reason to ask for that and not for std::string_view. And if so, then everything can work automatically without us making ugly #ifdefs: https://godbolt.org/z/Wc8Kd14Y8.

Signed-off-by: Byoungro So <[email protected]>

sycl/source/device.cpp

aelovikov-intel

Conditionally approving predicated on the following:

@sergey-semenov has to approve too in order for this to be merged
The very next PR should address MKernelName uses to reduce number of customizations using the strategy discussed in earlier comments and later offline.
The next PR after 2) would setup testing to ensure we don't have any other instances of std::string/std::string_view on the ABI boundary. Has to come after 2) so that the approach from 2) is used here.

Co-authored-by: aelovikov-intel <[email protected]>

sergey-semenov

LGTM considering the plan laid out in Andrei's comment, one extra minor comment that I'm fine with being addressed in the follow up PR.

sergey-semenov · 2024-03-01T16:15:44Z

sycl/include/sycl/kernel_bundle.hpp

 } // namespace detail

 /// \returns the kernel_id associated with the KernelName
 template <typename KernelName> kernel_id get_kernel_id() {
  // FIXME: This must fail at link-time if KernelName not in any available
  // translation units.
  using KI = sycl::detail::KernelInfo<KernelName>;
+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES


You should be able to drop the else branch in favor of this one.

Thanks a lot, I will address it in a follow-up PR.

bso-intel requested a review from a team as a code owner December 15, 2023 18:45

bso-intel requested a review from KseniyaTikhomirova December 15, 2023 18:45

bso-intel added 3 commits December 15, 2023 10:51

clang-format fix

4dabfd3

Signed-off-by: Byoungro So <[email protected]>

Merge remote-tracking branch 'upstream/sycl' into abi

108bf4d

clang-format

d7f21ab

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock December 15, 2023 19:41 — with GitHub Actions Failure

format

34cd0b9

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock December 15, 2023 19:59 — with GitHub Actions Failure

bso-intel had a problem deploying to WindowsCILock December 15, 2023 20:18 — with GitHub Actions Error

fix mockup

ded92db

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock December 15, 2023 20:23 — with GitHub Actions Failure

bso-intel had a problem deploying to WindowsCILock December 15, 2023 20:37 — with GitHub Actions Error

bso-intel added 2 commits December 15, 2023 12:46

fix test

77aaeba

Signed-off-by: Byoungro So <[email protected]>

fix test

fa6a931

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock December 15, 2023 20:51 — with GitHub Actions Failure

bso-intel had a problem deploying to WindowsCILock December 15, 2023 21:05 — with GitHub Actions Error

bso-intel marked this pull request as draft December 15, 2023 21:05

fix test

5709e28

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock December 15, 2023 21:08 — with GitHub Actions Failure

bso-intel had a problem deploying to WindowsCILock December 15, 2023 21:29 — with GitHub Actions Failure

bso-intel changed the title ~~[SYCL] Initial change for C++11 ABI=0 support~~ [SYCL] Initial changes for C++11 ABI=0 support Dec 17, 2023

bader reviewed Dec 18, 2023

View reviewed changes

sycl/include/sycl/device.hpp Outdated Show resolved Hide resolved

sycl/include/sycl/string.hpp Outdated Show resolved Hide resolved

sycl/include/sycl/string.hpp Outdated Show resolved Hide resolved

sycl/include/sycl/string.hpp Outdated Show resolved Hide resolved

address feedback

da8ab9f

Signed-off-by: Byoungro So <[email protected]>

clang-format

841f042

Signed-off-by: Byoungro So <[email protected]>

bso-intel had a problem deploying to WindowsCILock January 4, 2024 20:26 — with GitHub Actions Failure

bso-intel had a problem deploying to WindowsCILock January 4, 2024 20:59 — with GitHub Actions Failure

guard preview-break-changes

a79d58f

Signed-off-by: Byoungro So <[email protected]>

bso-intel temporarily deployed to WindowsCILock February 28, 2024 00:23 — with GitHub Actions Inactive

address feedback

0d19467

Signed-off-by: Byoungro So <[email protected]>

bso-intel temporarily deployed to WindowsCILock February 28, 2024 03:34 — with GitHub Actions Inactive

bso-intel had a problem deploying to WindowsCILock February 28, 2024 04:03 — with GitHub Actions Failure

bso-intel temporarily deployed to WindowsCILock February 28, 2024 04:07 — with GitHub Actions Inactive

aelovikov-intel reviewed Feb 28, 2024

View reviewed changes

sycl/source/device.cpp Outdated Show resolved Hide resolved

aelovikov-intel reviewed Feb 28, 2024

View reviewed changes

sycl/source/platform.cpp Outdated Show resolved Hide resolved

bso-intel had a problem deploying to WindowsCILock February 28, 2024 18:08 — with GitHub Actions Failure

aelovikov-intel reviewed Feb 28, 2024

View reviewed changes

bso-intel had a problem deploying to WindowsCILock February 28, 2024 18:52 — with GitHub Actions Failure

bso-intel force-pushed the abi branch from ec62b29 to 0d19467 Compare February 28, 2024 19:57

bso-intel temporarily deployed to WindowsCILock February 28, 2024 19:57 — with GitHub Actions Inactive

bso-intel had a problem deploying to WindowsCILock February 28, 2024 20:26 — with GitHub Actions Failure

keep both

85b7d36

Signed-off-by: Byoungro So <[email protected]>

bso-intel temporarily deployed to WindowsCILock February 28, 2024 20:47 — with GitHub Actions Inactive

bso-intel had a problem deploying to WindowsCILock February 28, 2024 21:17 — with GitHub Actions Failure

bso-intel temporarily deployed to WindowsCILock February 28, 2024 21:20 — with GitHub Actions Inactive

bso-intel had a problem deploying to WindowsCILock February 28, 2024 21:21 — with GitHub Actions Failure

bso-intel temporarily deployed to WindowsCILock February 28, 2024 21:33 — with GitHub Actions Inactive

bso-intel temporarily deployed to WindowsCILock February 28, 2024 21:34 — with GitHub Actions Inactive

aelovikov-intel reviewed Feb 29, 2024

View reviewed changes

sycl/source/device.cpp Outdated Show resolved Hide resolved

aelovikov-intel approved these changes Feb 29, 2024

View reviewed changes

bso-intel and others added 2 commits February 29, 2024 11:57

Update sycl/source/device.cpp

b79a347

Co-authored-by: aelovikov-intel <[email protected]>

Update sycl/include/sycl/device.hpp

1650d6d

Co-authored-by: aelovikov-intel <[email protected]>

bso-intel temporarily deployed to WindowsCILock February 29, 2024 20:45 — with GitHub Actions Inactive

bso-intel temporarily deployed to WindowsCILock February 29, 2024 21:13 — with GitHub Actions Inactive

sergey-semenov approved these changes Mar 1, 2024

View reviewed changes

sergey-semenov merged commit 459e122 into intel:sycl Mar 1, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bso-intel commented Dec 15, 2023 •

edited

Loading

bso-intel commented Dec 17, 2023

github-actions bot commented Jan 4, 2024 •

edited

Loading

aelovikov-intel Feb 28, 2024

bso-intel Feb 28, 2024

aelovikov-intel Feb 28, 2024

aelovikov-intel Feb 28, 2024

bso-intel Feb 28, 2024 •

edited

Loading

aelovikov-intel Feb 28, 2024

bso-intel Feb 28, 2024

aelovikov-intel Feb 28, 2024

aelovikov-intel left a comment

sergey-semenov left a comment •

edited

Loading

sergey-semenov Mar 1, 2024

bso-intel Mar 1, 2024

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

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

Conversation

bso-intel commented Dec 15, 2023 • edited Loading

bso-intel commented Dec 17, 2023

github-actions bot commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bso-intel Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelovikov-intel left a comment

Choose a reason for hiding this comment

sergey-semenov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bso-intel commented Dec 15, 2023 •

edited

Loading

github-actions bot commented Jan 4, 2024 •

edited

Loading

bso-intel Feb 28, 2024 •

edited

Loading

sergey-semenov left a comment •

edited

Loading