From d0a396b2b9c5a2be005c6c42caaadb72c66297ff Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Wed, 22 Jul 2020 19:00:57 +0300 Subject: [PATCH 1/6] [SYCL] Aligned set_arg behaviour with SYCL specification According to SYCL 1.2.1 specification, rev. 7, paragraph 4.8.5: template void set_arg(int argIndex, T &&arg) ... The argument can be either a SYCL accessor, a SYCL sampler or a trivially copyable and standard-layout C++ type. Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/handler.hpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sycl/include/CL/sycl/handler.hpp b/sycl/include/CL/sycl/handler.hpp index da60f8258a0bb..c928ec187b890 100644 --- a/sycl/include/CL/sycl/handler.hpp +++ b/sycl/include/CL/sycl/handler.hpp @@ -386,7 +386,11 @@ class __SYCL_EXPORT handler { static_cast(AccessTarget), ArgIndex); } - template void setArgHelper(int ArgIndex, T &&Arg) { + template + typename std::enable_if::value && + std::is_standard_layout::value, + void>::type + setArgHelper(int ArgIndex, T &&Arg) { void *StoredArg = (void *)storePlainArg(Arg); if (!std::is_same::value && std::is_pointer::value) { From 590ed41688d507de036e64216ffee8e95e4adabb Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Wed, 22 Jul 2020 19:49:23 +0300 Subject: [PATCH 2/6] Relaxed requirement for SYCL-2020's set_arg. Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/handler.hpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/sycl/include/CL/sycl/handler.hpp b/sycl/include/CL/sycl/handler.hpp index c928ec187b890..ac7c4c2eae34f 100644 --- a/sycl/include/CL/sycl/handler.hpp +++ b/sycl/include/CL/sycl/handler.hpp @@ -386,10 +386,16 @@ class __SYCL_EXPORT handler { static_cast(AccessTarget), ArgIndex); } + template struct ShouldEnableSetArgHelper { + static constexpr bool value = std::is_trivially_copyable::value +#if CL_SYCL_LANGUAGE_VERSION <= 121 + && std::is_standard_layout::value +#endif + ; + }; + template - typename std::enable_if::value && - std::is_standard_layout::value, - void>::type + typename std::enable_if::value, void>::type setArgHelper(int ArgIndex, T &&Arg) { void *StoredArg = (void *)storePlainArg(Arg); From 7e999865c7e70b1c241c06bb6d37b4c7eab886f5 Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Wed, 22 Jul 2020 20:02:52 +0300 Subject: [PATCH 3/6] Small fix Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/handler.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sycl/include/CL/sycl/handler.hpp b/sycl/include/CL/sycl/handler.hpp index ac7c4c2eae34f..14d91e8865a35 100644 --- a/sycl/include/CL/sycl/handler.hpp +++ b/sycl/include/CL/sycl/handler.hpp @@ -388,8 +388,10 @@ class __SYCL_EXPORT handler { template struct ShouldEnableSetArgHelper { static constexpr bool value = std::is_trivially_copyable::value +#ifdef CL_SYCL_LANGUAGE_VERSION #if CL_SYCL_LANGUAGE_VERSION <= 121 && std::is_standard_layout::value +#endif #endif ; }; From 9a16b666c8a62398523a7a6644ff231b185b7fb9 Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Sat, 25 Jul 2020 20:56:18 +0300 Subject: [PATCH 4/6] Added a test and reworked solution Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/handler.hpp | 42 ++++++++++++++-------- sycl/test/basic_tests/set_arg_error.cpp | 47 +++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 sycl/test/basic_tests/set_arg_error.cpp diff --git a/sycl/include/CL/sycl/handler.hpp b/sycl/include/CL/sycl/handler.hpp index 14d91e8865a35..76599247f5dc9 100644 --- a/sycl/include/CL/sycl/handler.hpp +++ b/sycl/include/CL/sycl/handler.hpp @@ -386,19 +386,7 @@ class __SYCL_EXPORT handler { static_cast(AccessTarget), ArgIndex); } - template struct ShouldEnableSetArgHelper { - static constexpr bool value = std::is_trivially_copyable::value -#ifdef CL_SYCL_LANGUAGE_VERSION -#if CL_SYCL_LANGUAGE_VERSION <= 121 - && std::is_standard_layout::value -#endif -#endif - ; - }; - - template - typename std::enable_if::value, void>::type - setArgHelper(int ArgIndex, T &&Arg) { + template void setArgHelper(int ArgIndex, T &&Arg) { void *StoredArg = (void *)storePlainArg(Arg); if (!std::is_same::value && std::is_pointer::value) { @@ -808,13 +796,39 @@ class __SYCL_EXPORT handler { } } + template + using remove_cv_ref_t = + typename std::remove_cv>::type; + + template struct ShouldEnableSetArg { + static constexpr bool value = + std::is_trivially_copyable::value +#if CL_SYCL_LANGUAGE_VERSION && CL_SYCL_LANGUAGE_VERSION <= 121 + && std::is_standard_layout::value +#endif + || std::is_same>::value // Sampler + || (!std::is_same>::value && + std::is_pointer>::value) // USM + || std::is_same>::value; // Interop + }; + /// Sets argument for OpenCL interoperability kernels. /// /// Registers Arg passed as argument # ArgIndex. /// /// \param ArgIndex is a positional number of argument to be set. /// \param Arg is an argument value to be set. - template void set_arg(int ArgIndex, T &&Arg) { + template + typename std::enable_if::value, void>::type + set_arg(int ArgIndex, T &&Arg) { + setArgHelper(ArgIndex, std::move(Arg)); + } + + template + void + set_arg(int ArgIndex, + accessor Arg) { setArgHelper(ArgIndex, std::move(Arg)); } diff --git a/sycl/test/basic_tests/set_arg_error.cpp b/sycl/test/basic_tests/set_arg_error.cpp new file mode 100644 index 0000000000000..70fe7d4e2948a --- /dev/null +++ b/sycl/test/basic_tests/set_arg_error.cpp @@ -0,0 +1,47 @@ +// RUN: %clangxx -fsycl -Xclang -verify %s -I %sycl_include -Xclang -verify-ignore-unexpected=note,warning -fsyntax-only + +#include + +struct TriviallyCopyable { + int a; + int b; +}; + +struct NonTriviallyCopyable { + NonTriviallyCopyable() = default; + NonTriviallyCopyable(NonTriviallyCopyable const &) {} + int a; + int b; +}; + +struct NonStdLayout { + int a; + +private: + int b; +}; + +int main() { + constexpr size_t size = 1; + cl::sycl::buffer buf(size); + cl::sycl::queue q; + q.submit([&](cl::sycl::handler &h) { + auto global_acc = buf.get_access(h); + cl::sycl::sampler samp(cl::sycl::coordinate_normalization_mode::normalized, + cl::sycl::addressing_mode::clamp, + cl::sycl::filtering_mode::nearest); + cl::sycl::accessor + local_acc({size}, h); + h.set_arg(0, local_acc); + h.set_arg(1, global_acc); + h.set_arg(2, samp); + h.set_arg(3, TriviallyCopyable{}); + h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} + 5, NonTriviallyCopyable{}); +#ifdef CL_SYCL_LANGUAGE_VERSION &&CL_SYCL_LANGUAGE_VERSION <= 121 + h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} + 4, NonStdLayout{}); +#endif + }); +} From 8c037949da03b200afffe091b9df53d95b8c92a2 Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Mon, 27 Jul 2020 12:56:38 +0300 Subject: [PATCH 5/6] Fixed small typo Signed-off-by: Ivan Karachun --- sycl/test/basic_tests/set_arg_error.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/set_arg_error.cpp b/sycl/test/basic_tests/set_arg_error.cpp index 70fe7d4e2948a..b57b9fb969cf0 100644 --- a/sycl/test/basic_tests/set_arg_error.cpp +++ b/sycl/test/basic_tests/set_arg_error.cpp @@ -39,7 +39,7 @@ int main() { h.set_arg(3, TriviallyCopyable{}); h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} 5, NonTriviallyCopyable{}); -#ifdef CL_SYCL_LANGUAGE_VERSION &&CL_SYCL_LANGUAGE_VERSION <= 121 +#ifdef CL_SYCL_LANGUAGE_VERSION && CL_SYCL_LANGUAGE_VERSION <= 121 h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} 4, NonStdLayout{}); #endif From 84a7b85a5e4e378f9830c310f8eb1066ce21905a Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Mon, 27 Jul 2020 20:04:36 +0300 Subject: [PATCH 6/6] Refactored ShoudlEnableSetArg and fixed the test (again) Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/handler.hpp | 11 +++++++---- sycl/test/basic_tests/set_arg_error.cpp | 6 +++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/sycl/include/CL/sycl/handler.hpp b/sycl/include/CL/sycl/handler.hpp index 76599247f5dc9..982250a52879b 100644 --- a/sycl/include/CL/sycl/handler.hpp +++ b/sycl/include/CL/sycl/handler.hpp @@ -800,16 +800,19 @@ class __SYCL_EXPORT handler { using remove_cv_ref_t = typename std::remove_cv>::type; + template + using is_same_type = std::is_same, remove_cv_ref_t>; + template struct ShouldEnableSetArg { static constexpr bool value = std::is_trivially_copyable::value #if CL_SYCL_LANGUAGE_VERSION && CL_SYCL_LANGUAGE_VERSION <= 121 && std::is_standard_layout::value #endif - || std::is_same>::value // Sampler - || (!std::is_same>::value && - std::is_pointer>::value) // USM - || std::is_same>::value; // Interop + || is_same_type::value // Sampler + || (!is_same_type::value && + std::is_pointer>::value) // USM + || is_same_type::value; // Interop }; /// Sets argument for OpenCL interoperability kernels. diff --git a/sycl/test/basic_tests/set_arg_error.cpp b/sycl/test/basic_tests/set_arg_error.cpp index b57b9fb969cf0..4472b50bcd669 100644 --- a/sycl/test/basic_tests/set_arg_error.cpp +++ b/sycl/test/basic_tests/set_arg_error.cpp @@ -38,10 +38,10 @@ int main() { h.set_arg(2, samp); h.set_arg(3, TriviallyCopyable{}); h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} - 5, NonTriviallyCopyable{}); -#ifdef CL_SYCL_LANGUAGE_VERSION && CL_SYCL_LANGUAGE_VERSION <= 121 + 4, NonTriviallyCopyable{}); +#if CL_SYCL_LANGUAGE_VERSION && CL_SYCL_LANGUAGE_VERSION <= 121 h.set_arg( // expected-error {{no matching member function for call to 'set_arg'}} - 4, NonStdLayout{}); + 5, NonStdLayout{}); #endif }); }