[SYCL] Extends support for SYCL 2020 implicitly device copyable types #8195

maarquitos14 · 2023-02-03T10:36:40Z

Extends support for SYCL2020 implicitly device copyable types and adds new tests to check correct behavior.

Signed-off-by: Maronas, Marcos <[email protected]>

aelovikov-intel · 2023-02-03T16:52:22Z

sycl/include/sycl/types.hpp

+// std::variant<> is implicitly device copyable type
+template <> struct is_device_copyable<std::variant<>> : std::true_type {};
+
+// std::variant<Ts...> is implicitly device copyable type if each type T of
+// Ts... is device copyable, and it is not trivially copyable (if it is
+// trivially copyable it is device copyable by default)
+template <typename T, typename... Ts>
+struct is_device_copyable<std::variant<T, Ts...>,
+                          std::enable_if_t<!std::is_trivially_copyable<
+                              std::variant<T, Ts...>>::value>>
+    : detail::bool_constant<is_device_copyable<T>::value &&
+                            is_device_copyable<std::variant<Ts...>>::value> {};
+


I'd prefer it written using fold expressions.

aelovikov-intel · 2023-02-03T16:55:46Z

sycl/test/basic_tests/implicit_device_copyable_types.cpp

@@ -0,0 +1,161 @@
+// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out


I'd like to have more runtime checks that the values of those types actually match between host/device, i.e. check the content of pair.first and/or pair.second.

Just to clarify - this is my main concern, two other comments are more subjective in nature and thus optional to implement.

I suggest that we perhaps have two test cases: one which simply contains a bunch of static_assert(sycl::is_device_copyable_v<some_type_here>) to check that we properly specialize the trait and another one, which checks runtime behavior, i.e. that we are actually able to properly copy values of those types to kernels and read them back.

I have submitted a PR to llvm-test-suite repo adding tests that check the runtime behavior for this.

If this test is supposed to be compile-only, then I suggest to remove -o %t.out and add -fsyntax-only, see docs for some BKMs. You can also remove any code which submits kernels, because that will be checked in the E2E test you have

aelovikov-intel · 2023-02-03T16:58:02Z

sycl/test/basic_tests/implicit_device_copyable_types.cpp

+         // std::variant with complex types relies on virtual functions, so
+         // they cannot be used within sycl kernels


Do we verify it anywhere? If not, I'd suggest to modify this test as

// RUN: compile and run // RUN: compile-only -fsyntax-only -Xclang <enable error/warning verification in clang> -DCOMPILE_ONLY ... #if COMPILE_ONLY { std::variant<ACopyable> // try to pass it to device // expected-error: ... } #endif

I developed this test and tried both in Linux and Windows. Turns out there's no error in Windows (I guess MSVC doesn't require virtual functions to implement std::variant), so I don't think we should have this test. Also, I will update my comment to say that this only happens in some implementations of std::variant.

sycl/include/sycl/types.hpp

AlexeySachkov · 2023-02-06T11:32:27Z

sycl/test/basic_tests/implicit_device_copyable_types.cpp

@@ -0,0 +1,161 @@
+// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out


I suggest that we perhaps have two test cases: one which simply contains a bunch of static_assert(sycl::is_device_copyable_v<some_type_here>) to check that we properly specialize the trait and another one, which checks runtime behavior, i.e. that we are actually able to properly copy values of those types to kernels and read them back.

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 · 2023-02-09T14:28:13Z

/verify with intel/llvm-test-suite#1583

…t-device-copyable-types

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 · 2023-02-13T11:10:06Z

/verify with intel/llvm-test-suite#1583

maarquitos14 · 2023-02-13T16:07:12Z

/verify with intel/llvm-test-suite#1583

aelovikov-intel · 2023-02-13T17:24:11Z

@AlexeySachkov is out of office but he marked all his questions/requests as resolved, so I am going LGTM this one.

@AlexeySachkov , please finish your review post-commit once you're back.

maarquitos14 · 2023-02-14T15:47:32Z

/verify with intel/llvm-test-suite#1583

maarquitos14 added 3 commits February 3, 2023 02:32

Extend support for implicitly device copyable types

cbf6abe

Signed-off-by: Maronas, Marcos <[email protected]>

Adds tests for implicit device copyable types.

cd4b93d

Signed-off-by: Maronas, Marcos <[email protected]>

Extend support for SYCL2020 implicitly device copyable types.

6b02640

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 requested a review from a team as a code owner February 3, 2023 10:36

maarquitos14 requested a review from aelovikov-intel February 3, 2023 10:36

maarquitos14 added 2 commits February 3, 2023 03:29

Fix clang-format issues.

80c8fc6

Signed-off-by: Maronas, Marcos <[email protected]>

Fix more clang-format issues.

e36f4b8

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 3, 2023 12:09 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws February 3, 2023 12:41 — with GitHub Actions Inactive

aelovikov-intel reviewed Feb 3, 2023

View reviewed changes

AlexeySachkov reviewed Feb 6, 2023

View reviewed changes

Addressing code review comments.

7fcac67

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 8, 2023 18:04 — with GitHub Actions Inactive

maarquitos14 added 4 commits February 9, 2023 05:58

Extends is_device_copyable to consider cv-qualifiers.

61c6e12

Signed-off-by: Maronas, Marcos <[email protected]>

Extends testing with cv-qualifiers.

918c61a

Signed-off-by: Maronas, Marcos <[email protected]>

Undoing unnecessary comment.

8804479

Signed-off-by: Maronas, Marcos <[email protected]>

Fixes clang-format issues.

290c72e

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 9, 2023 14:38 — with GitHub Actions Inactive

maarquitos14 added 2 commits February 9, 2023 06:52

Merge remote-tracking branch 'intel/origin/sycl' into maronas/implici…

982208a

…t-device-copyable-types

Recovers accidentally removed line.

10913de

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 9, 2023 16:42 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws February 9, 2023 19:17 — with GitHub Actions Inactive

AlexeySachkov requested a review from aelovikov-intel February 10, 2023 08:22

Corrects test behaviour.

ad1a710

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 10, 2023 10:45 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws February 10, 2023 11:17 — with GitHub Actions Inactive

maarquitos14 added 2 commits February 10, 2023 06:20

Fixes issues with MSVC.

d8e5c39

Signed-off-by: Maronas, Marcos <[email protected]>

Removes incorrect test.

7e61201

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 10, 2023 19:51 — with GitHub Actions Inactive

Removes unnecessary flags from test.

c82955e

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws February 13, 2023 09:57 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws February 13, 2023 10:41 — with GitHub Actions Inactive

aelovikov-intel approved these changes Feb 13, 2023

View reviewed changes

bader changed the title ~~[SYCL] Extends support for SYCL2020 implicitly device copyable types~~ [SYCL] Extends support for SYCL 2020 implicitly device copyable types Feb 15, 2023

bader merged commit bad3b38 into intel:sycl Feb 15, 2023

masterleinad mentioned this pull request Oct 19, 2023

SYCL: Restrict workaround for is_device_copyable to oneAPI versions before 2024.0.0 kokkos/kokkos#6532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Extends support for SYCL 2020 implicitly device copyable types #8195

[SYCL] Extends support for SYCL 2020 implicitly device copyable types #8195

Uh oh!

maarquitos14 commented Feb 3, 2023

Uh oh!

aelovikov-intel Feb 3, 2023

Uh oh!

aelovikov-intel Feb 3, 2023

Uh oh!

aelovikov-intel Feb 3, 2023

Uh oh!

AlexeySachkov Feb 6, 2023 •

edited

Loading

Uh oh!

maarquitos14 Feb 8, 2023

Uh oh!

AlexeySachkov Feb 9, 2023

Uh oh!

aelovikov-intel Feb 3, 2023

Uh oh!

maarquitos14 Feb 10, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov Feb 6, 2023 •

edited

Loading

Uh oh!

maarquitos14 commented Feb 9, 2023

Uh oh!

maarquitos14 commented Feb 13, 2023

Uh oh!

maarquitos14 commented Feb 13, 2023

Uh oh!

aelovikov-intel commented Feb 13, 2023

Uh oh!

maarquitos14 commented Feb 14, 2023

Uh oh!

Uh oh!

		@@ -0,0 +1,161 @@
		// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out

		// std::variant with complex types relies on virtual functions, so
		// they cannot be used within sycl kernels

[SYCL] Extends support for SYCL 2020 implicitly device copyable types #8195

[SYCL] Extends support for SYCL 2020 implicitly device copyable types #8195

Uh oh!

Conversation

maarquitos14 commented Feb 3, 2023

Uh oh!

aelovikov-intel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Feb 6, 2023 • 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!

maarquitos14 Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

maarquitos14 Feb 10, 2023 • 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!

Uh oh!

Uh oh!

AlexeySachkov Feb 6, 2023 • 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!

maarquitos14 commented Feb 9, 2023

Uh oh!

maarquitos14 commented Feb 13, 2023

Uh oh!

maarquitos14 commented Feb 13, 2023

Uh oh!

aelovikov-intel commented Feb 13, 2023

Uh oh!

maarquitos14 commented Feb 14, 2023

Uh oh!

Uh oh!

AlexeySachkov Feb 6, 2023 •

edited

Loading

maarquitos14 Feb 10, 2023 •

edited

Loading

AlexeySachkov Feb 6, 2023 •

edited

Loading