[SYCL] Use C-arrays as storage in `sycl::vec` #17025

uditagarwal97 · 2025-02-14T18:36:44Z

#14130 caused sycl::vec to use std::array as its underlying storage. However, operations on std::array may emit debug-mode-only functions, on which the device compiler may fail.

This PR replaces std::array with C-style arrays as storage in sycl::vec.

sycl/test/regression/vec_array_windows.cpp

sycl/test/abi/layout_vec.cpp

uditagarwal97 · 2025-02-24T16:39:43Z

@aelovikov-intel @steffenlarsen ping. Please approve if changes looks good.

steffenlarsen · 2025-02-25T05:47:37Z

sycl/include/sycl/detail/vector_convert.hpp

@@ -930,7 +933,11 @@ vec<convertT, NumElements> vec<DataT, NumElements>::convert() const {
      auto val =
          detail::convertImpl<T, R, roundingMode, NumElements, OpenCLVecT,
                              OpenCLVecR>(bit_cast<OpenCLVecT>(*this));
+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
+      sycl::detail::memcpy_no_adl(&Result.m_Data, &val, sizeof(Result));


Would there be a problem in just switching to memcpy_no_adl for all paths?

We can, but I don't see any improvement with that. If we were to do that, we would have something like the following which doesn't simplify the code.

#ifdef __INTEL_PREVIEW_BREAKING_CHANGES sycl::detail::memcpy_no_adl(&Result.m_Data, &val, sizeof(Result)); // For C-array #else sycl::detail::memcpy_no_adl(Result.m_Data.data(), &val, sizeof(Result)); // For std::array #endif

We couldn't have done sycl::detail::memcpy_no_adl(&Result.m_Data, &val, sizeof(Result)); for std::array as base address of std::array is not guaranteed by the standard to be same as that of underlying C-array: https://stackoverflow.com/questions/69500721/is-the-address-of-a-stdarray-guaranteed-the-same-as-its-data

On a second though, as SO answer mentioned, most popular implementations does guarantee std::array and underlying C-array to have same address. So, I can do something like:

static_assert(sizeof(Result.m_Data) == sizeof(DataT[AdjustedNum])) sycl::detail::memcpy_no_adl(&Result.m_Data, &val, sizeof(Result)); // for both C-array and std::array

I think having the static_assert should be fine. We only risk this hitting weird implementations until preview changes are promoted, so I think the risk is low while the reduced complexity is nice. 😄

sycl/include/sycl/detail/vector_convert.hpp

steffenlarsen · 2025-02-25T05:48:57Z

sycl/include/sycl/vector.hpp

@@ -287,7 +291,12 @@ class __SYCL_EBO vec
      typename vector_t_ = vector_t,
      typename = typename std::enable_if_t<std::is_same_v<vector_t_, vector_t>>>
  constexpr vec(vector_t_ openclVector) {
+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
+    sycl::detail::memcpy_no_adl(&this->m_Data, &openclVector,
+                                sizeof(openclVector));


Same question as above.

aelovikov-intel · 2025-02-26T16:51:24Z

@steffenlarsen , please approve this. You were the one pushing to do this under preview, so you should be approving it.

steffenlarsen

Sorry, I hadn't noticed the recent changes. Looks good!

The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. intel#17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there)

The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. #17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there)

The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. intel#17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there) This is a cherry-pick of intel#17656

…ses" to sycl-rel-6_0_0 (#17697) This is a cherry-pick of #17656 + changes required to resolve merge conflicts. -------------------------------------- The problem with using std::array in sycl::vec is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. #17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: it allows to opt-in for new behavior through passing -D__SYCL_USE_NEW_VEC_IMPL=1 macro without using -fpreview-breaking-changes flag. That allows for a more gradual opt-in from customers who are interested in this fix it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding std::array - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there) --------- Co-authored-by: Alexey Sachkov <alexey.sachkov@intel.com>

The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. #17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there)

### Cherry-pick of: #17656 and some changes to resolve merge conflicts & make it work. The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. #17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there) Co-authored-by: Alexey Sachkov <alexey.sachkov@intel.com>

The problem with using `std::array` in `sycl::vec` is that we cannot compile it in some environments (namely, Windows) because the former may use something that is illegal in SYCL device code. #17025 fixed that, but only did so under preview breaking changes mode, which does not satisfy some of our customers immediately. This PR introduces two main changes: - it allows to opt-in for new behavior through passing `-D__SYCL_USE_NEW_VEC_IMPL=1` macro without using `-fpreview-breaking-changes` flag. That allows for a more gradual opt-in from customers who are interested in this fix - it switches the imlpementation to use the new approach with C-style arrays if their size and alignment is the same as for the corresponding `std::array` - in that case their memory layout is expected to be absolutely the same and therefore it should be safe to use the new approach without fear of some ABI incompatibilities. This allows for customers to benefit from the fix without specifying any extra macro (which should be the case for the most common platforms out there)

uditagarwal97 self-assigned this Feb 14, 2025

uditagarwal97 had a problem deploying to WindowsCILock February 14, 2025 18:38 — with GitHub Actions Failure

uditagarwal97 temporarily deployed to WindowsCILock February 14, 2025 19:49 — with GitHub Actions Inactive

uditagarwal97 had a problem deploying to WindowsCILock February 14, 2025 21:16 — with GitHub Actions Error

uditagarwal97 requested review from AlexeySachkov and aelovikov-intel February 14, 2025 21:18

uditagarwal97 marked this pull request as ready for review February 14, 2025 21:18

uditagarwal97 requested a review from a team as a code owner February 14, 2025 21:18

uditagarwal97 requested a review from steffenlarsen February 14, 2025 21:18

aelovikov-intel reviewed Feb 14, 2025

View reviewed changes

sycl/test/regression/vec_array_windows.cpp Outdated Show resolved Hide resolved

uditagarwal97 temporarily deployed to WindowsCILock February 14, 2025 21:30 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock February 14, 2025 22:01 — with GitHub Actions Inactive

steffenlarsen reviewed Feb 17, 2025

View reviewed changes

sycl/test/abi/layout_vec.cpp Outdated Show resolved Hide resolved

uditagarwal97 force-pushed the vec_c_array branch from 4f1c4d6 to 74316f2 Compare February 19, 2025 20:16

uditagarwal97 temporarily deployed to WindowsCILock February 19, 2025 20:17 — with GitHub Actions Inactive

uditagarwal97 requested a review from steffenlarsen February 19, 2025 20:17

uditagarwal97 temporarily deployed to WindowsCILock February 19, 2025 21:15 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock February 24, 2025 16:40 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock February 24, 2025 19:23 — with GitHub Actions Inactive

steffenlarsen reviewed Feb 25, 2025

View reviewed changes

uditagarwal97 added 2 commits February 25, 2025 09:38

Put changes under preview-breaking flag.

b5b18a5

Address reviews

71ae31d

uditagarwal97 force-pushed the vec_c_array branch from c13708f to 71ae31d Compare February 25, 2025 20:55

uditagarwal97 had a problem deploying to WindowsCILock February 25, 2025 20:56 — with GitHub Actions Error

Fix windows test

f128407

uditagarwal97 temporarily deployed to WindowsCILock February 25, 2025 20:57 — with GitHub Actions Inactive

uditagarwal97 requested a review from steffenlarsen February 25, 2025 22:43

uditagarwal97 temporarily deployed to WindowsCILock February 26, 2025 01:20 — with GitHub Actions Inactive

uditagarwal97 requested a review from aelovikov-intel February 26, 2025 16:31

steffenlarsen approved these changes Feb 27, 2025

View reviewed changes

steffenlarsen merged commit 47404a4 into intel:sycl Feb 27, 2025
20 checks passed

AlexeySachkov mentioned this pull request Mar 26, 2025

[SYCL] Switch to use plain array in sycl::vec in more cases #17656

Merged

This was referenced Mar 27, 2025

[SYCL] Cherry-pick "Switch to use plain array in sycl::vec in more cases" to sycl-rel-6_0_0 #17695

Closed

[SYCL] Cherry-pick "Switch to use plain array in sycl::vec in more cases" to sycl-rel-6_0_0 #17697

Merged

KornevNikita mentioned this pull request Apr 2, 2025

[SYCL] Switch to use plain array in sycl::vec in more cases (#17656) #17804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Use C-arrays as storage in `sycl::vec` #17025

[SYCL] Use C-arrays as storage in `sycl::vec` #17025

Uh oh!

uditagarwal97 commented Feb 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

uditagarwal97 commented Feb 24, 2025

Uh oh!

steffenlarsen Feb 25, 2025

Uh oh!

uditagarwal97 Feb 25, 2025 •

edited

Loading

Uh oh!

uditagarwal97 Feb 25, 2025

Uh oh!

steffenlarsen Feb 26, 2025

Uh oh!

Uh oh!

steffenlarsen Feb 25, 2025

Uh oh!

aelovikov-intel commented Feb 26, 2025

Uh oh!

steffenlarsen left a comment

Uh oh!

Uh oh!

Uh oh!

[SYCL] Use C-arrays as storage in sycl::vec #17025

[SYCL] Use C-arrays as storage in sycl::vec #17025

Uh oh!

Conversation

uditagarwal97 commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uditagarwal97 commented Feb 24, 2025

Uh oh!

steffenlarsen Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

uditagarwal97 Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uditagarwal97 Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steffenlarsen Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Feb 26, 2025

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[SYCL] Use C-arrays as storage in `sycl::vec` #17025

[SYCL] Use C-arrays as storage in `sycl::vec` #17025

uditagarwal97 commented Feb 14, 2025 •

edited

Loading

uditagarwal97 Feb 25, 2025 •

edited

Loading