Various smaller bugfixes in tests #870

fknorr · 2024-02-17T15:53:05Z

This PR fixes a bunch of discrepancies with the SYCL spec and minor bugs and ambiguities that surfaced when we attempted to build the CTS against SimSYCL.

Some of these issues might not be visible when building against a SYCL implementation (such as DPC++) that implements non-standard type conversions, member functions or other behavior.

In individual commits:

test_accessor: get_multi_ptr() is only defined for device- and local accessors
test_accessor_legacy: legacy accessors do not have member function size()
test_atomic_ref: Typo in function name memory_scope_is_suppoted
test_atomic_ref_stress: id<1> == int is ambiguous
test_device_info: info::device::image_max_array_size is not part of the Spec
test_device_info: info::device::opencl_c_version is not part of the Spec
test_device_selector: sycl::runtime_error is not part of the Spec
test_exceptions: sycl::exception is not default-constructible
~~test_hierarchical: sycl::id is not convertible to sycl::range~~ (adressed by rebase)
test_id: sycl::id<1> - 1 is ambiguous
test_kernel: remove template from non-dependent expression
test_marray: marray<bool> does not have operator++/--
test_namespace: Avoid multiple definitions of kernel names
test_scalars: Regex typo in CMakeLists.txt
test_vector_load_store: sycl::vec cannot load/store from accessor or load from non-const multi_ptr
test_vector_operators: vec<bool> does not provide operator++/--
util: Call to erase_if is ambiguous with C++20

CLAassistant · 2024-02-17T15:53:10Z

All committers have signed the CLA.

steffenlarsen · 2024-02-19T17:00:24Z

Thank you for going through all these bugs and addressing them! It makes me wonder if some of these should be part of the spec, simply for usability.
@gmlueck @AerialMantis @keryell @illuhad @tomdeakin - Tagging for awareness.

keryell

What a ride!

keryell · 2024-02-22T02:19:42Z

tests/accessor/generic_accessor_api_common.h

@@ -58,8 +58,23 @@ void test_accessor_methods(const AccT &accessor,
 #endif
 }

+template<typename Accessor>
+extern const sycl::target accessor_target_v;


Why this extern definition?

It's the base template that's being specialized below. For a variable template marking it extern allows me to omit the (invalid) definition and cause a compiler error when it's used in a constexpr context.

Why not. :-)

keryell · 2024-02-22T02:23:36Z

tests/accessor_legacy/accessor_api_local_common.h

-            fail_for_accessor<T, dims, mode, target>(log, typeName,
-                "accessor does not return the correct count");
-          }
-        }


What is the problem here?

This tests the legacy accessor<target::local> specialization, which does not have a size() member.

keryell · 2024-02-22T02:24:52Z

tests/atomic_ref_stress/atomic_ref_stress_common.h

@@ -172,15 +172,15 @@ class aquire_release {
          refA.store(0);
          refB.store(0);
          sycl::group_barrier(item.get_group());
-          if (item.get_local_id() == 0) {
+          if (item.get_local_id() == sycl::id(0)) {


I do not think so, the constructors are implicit https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#id-class

And that is half of the problem, because id is also implicitly convertible to size_t. See godbolt. If you switch to gcc and member operation you will get a bit more detailed error message. But I was also surprised by this, apparently I'm not enough familiar with those overload resolution rules.

In our implementation we use some hackery to avoid ambiguity here

Same here https://github.com/triSYCL/triSYCL/blob/master/include/triSYCL/detail/small_array.hpp#L381

So either the special-casing needs to be in the spec, or this explicit construction is correct.

keryell · 2024-02-22T02:25:18Z

tests/atomic_ref_stress/atomic_ref_stress_common.h

            x = refA.load();
            refB.store(1);
          } else {
            y = refB.load();
            refA.store(1);
          }
          sycl::group_barrier(item.get_group());
-          if (item.get_local_id() == 0)
+          if (item.get_local_id() == sycl::id(0))


keryell · 2024-02-22T02:27:06Z

tests/exceptions/exceptions_constructors.cpp

@@ -194,8 +194,14 @@ TEST_CASE("Constructors for sycl::exception with sycl::errc error codes",
      check_exception(copy, errcode, sycl::sycl_category());
    }
    SECTION("operator=(const exception& other)") {
+      // sycl::exception is not default-constructible, so explicitly construct


Good point!

keryell · 2024-02-22T02:32:06Z

tests/hierarchical/hierarchical_non_uniform_local_range.cpp

+static sycl::range<1> id_to_range(sycl::id<1> id) {
+  return sycl::range<1>(id[0]);
+}
+static sycl::range<2> id_to_range(sycl::id<2> id) {


Suggested change

static sycl::range<2> id_to_range(sycl::id<2> id) {

static sycl::range<2> to_range(sycl::id<2> id) {

since the id is clear as the parameter?

This is obsolete now because it has been fixed on SYCL-2020 branch in the meantime. Removed entirely.

keryell · 2024-02-22T02:34:49Z

tests/id/id.cpp

@@ -239,7 +239,8 @@ TEMPLATE_TEST_CASE_SIG("id can be implicitly conversion-constructed from item",
  q.submit([r, &result_buf](sycl::handler& cgh) {
     sycl::accessor result{result_buf, cgh, sycl::write_only};
     cgh.parallel_for<kernel_id<D>>(r, [=](sycl::item<D> itm) {
-       if (itm.get_id() == id<D>{r} - 1) {
+       // `ul` suffix is necessary to resolve ambiguity for id<1>
+       if (itm.get_id() == id<D>{r} - 1ul) {


Why l?
Just u does not work?
The real type would be uz but it is C++23.

Not sure, but size_t is at least unsigned long on all relevant architectures, no?

keryell · 2024-02-22T02:38:07Z

tests/marray/marray_operators.h

-        named_type_pack<op_upos, op_uneg, op_pre_inc, op_pre_dec, op_lnot,
-                        op_bnot>::generate("unary +", "unary -", "pre ++",
-                                           "pre --", "!", "~");
+    static const auto unary_operators = make_unary_operators();


Isn't it a good opportunity for a lambda with immediate invocation?

keryell · 2024-02-22T02:39:50Z

tests/marray/marray_operators.h

+    if (!std::is_same_v<DataT, bool>) {
+      static const auto unary_post_operators =


Suggested change

if (!std::is_same_v<DataT, bool>) {

static const auto unary_post_operators =

if constexpr (!std::is_same_v<DataT, bool>) {

constexpr auto unary_post_operators =

static const must remain because it's not a literal type (clang error)

keryell · 2024-02-22T02:44:12Z

tests/vector_operators/generate_vector_operators.py

-    }
+  // The standard does not require ++ and -- (either prefix or postfix) for the
+  // bool type, so we skip tests for these operators if DataT is bool. This cannot
+  // be done via `if constexpr` because the type is not a dependent type.


Isn't it a design bug of the test relying too much on Python to do the metaprogramming instead of leaving the instantiation to C++?

tomdeakin · 2024-02-29T17:32:36Z

The SYCL WG discussed this, and the review process should continue to merge this. We will then want to follow up on the new features proposed to possibly include in the spec. So, please continue to review and update this PR based on those reviews. Thanks!

bader · 2024-02-29T17:33:42Z

It makes me wonder if some of these should be part of the spec, simply for usability.

@steffenlarsen, could you open issues in SYCL-Docs project for the items which you think should be part of the spec, please?

steffenlarsen · 2024-03-07T14:33:19Z

It makes me wonder if some of these should be part of the spec, simply for usability.

@steffenlarsen, could you open issues in SYCL-Docs project for the items which you think should be part of the spec, please?

Opened KhronosGroup/SYCL-Docs#540 and KhronosGroup/SYCL-Docs#539.

As mentioned in the CTS pull request KhronosGroup/SYCL-CTS#870, two device info descriptors, namely `info::device::image_max_array_size` and `info::device::opencl_c_version`, have been deprecated and no longer exist in the SYCL 2020 specification. But they still exist in our implementation, so I'm setting up deprecation warnings for them. Also a related test case has been revised to remove the usage of `info::device::opencl_c_version`. Also `info::device::usm_system_allocator` has been replaced and marked as deprecated 2+ years ago in PR #4007 and #9217 respectively. So I'm cleaning up a few relevant code pieces in our code as well in this PR. --------- Signed-off-by: Hu, Peisen <peisen.hu@intel.com>

… multi_ptr

fknorr · 2024-06-13T10:47:47Z

Rebased, which allowed me to drop one commit whose problem has been addressed on the target branch in the meantime.

keryell

Thanks!

keryell

Thanks!

keryell · 2024-06-18T03:23:49Z

tests/accessor/generic_accessor_api_common.h

@@ -58,8 +58,23 @@ void test_accessor_methods(const AccT &accessor,
 #endif
 }

+template<typename Accessor>
+extern const sycl::target accessor_target_v;


Why not. :-)

bader · 2024-06-18T15:40:34Z

@fknorr, please, fix the formatting check.

fknorr requested a review from a team as a code owner February 17, 2024 15:53

fknorr force-pushed the various-fixes branch from 7319f88 to dad69ea Compare February 17, 2024 17:53

fknorr mentioned this pull request Feb 17, 2024

Add support for SimSYCL as a SYCL implementation #871

Open

fknorr force-pushed the various-fixes branch from dad69ea to f519819 Compare February 17, 2024 19:27

bader added the Agenda To be discussed during a SYCL committee meeting label Feb 20, 2024

psalz mentioned this pull request Feb 21, 2024

Rename hipSYCL to AdaptiveCpp, update CI and regenerate filter #874

Open

keryell reviewed Feb 22, 2024

View reviewed changes

bader removed the Agenda To be discussed during a SYCL committee meeting label Feb 29, 2024

This was referenced Mar 7, 2024

id<1> operators and integer literals KhronosGroup/SYCL-Docs#539

Open

Load on vec does not allow pointer to non-const data. KhronosGroup/SYCL-Docs#540

Open

gmlueck mentioned this pull request Mar 25, 2024

Allow "vec::load" / "vec::store" from/to an accessor KhronosGroup/SYCL-Docs#547

Open

HPS-1 mentioned this pull request Apr 4, 2024

[SYCL] Setup deprecation warnings for device info descriptors that have been deprecated and not part SYCL 2020 intel/llvm#13279

Merged

fknorr added 12 commits June 13, 2024 12:32

Fix: get_multi_ptr() is only defined for device- and local accessors

0f2a228

Fix: legacy accessors do not have member function size()

363c209

Fix: Typo in function name

e6aed12

Fix: id<1> == int is ambiguous

13b2277

Fix: info::device::image_max_array_size is not part of the Spec

ad599db

Fix: info::device::opencl_c_version is not part of the Spec

53fd92f

Fix: sycl::runtime_error is not part of the Spec

05c1a78

Fix: sycl::exception is not default-constructible

cb520fd

Fix: sycl::id<1> - 1 is ambiguous

9fe6b29

Fix: remove template from non-dependent expression

52f993b

Fix: marray<bool> does not have operator++/--

f9f2894

Fix: Avoid multiple definitions of kernel names

54d8b79

fknorr added 6 commits June 13, 2024 12:33

Fix: Regex typo in scalars/CMakeLists

502a9bf

Fix: sycl::vec cannot load/store from accessor or load from non-const…

9008312

… multi_ptr

Fix: vec<bool> does not provide operator++/--

0fbac0d

Fix: Call to erase_if is ambiguous with C++20

e931257

Fix: correctly reset array in test_scalars

8764459

Address reviewer comments on fixes

8a68532

fknorr force-pushed the various-fixes branch from b00d089 to 8a68532 Compare June 13, 2024 10:36

fknorr mentioned this pull request Jun 13, 2024

Compatibility: Only use sycl::half if SYCL_CTS_ENABLE_HALF_TESTS is set #872

Open

bader requested a review from keryell June 13, 2024 14:50

keryell approved these changes Jun 18, 2024

View reviewed changes

fknorr and others added 2 commits June 19, 2024 11:50

Fix formatting in updated tests

004bcbf

Merge branch 'SYCL-2020' into various-fixes

7d63aba

bader merged commit c9bd94a into KhronosGroup:SYCL-2020 Jun 20, 2024
8 checks passed

fknorr deleted the various-fixes branch June 20, 2024 16:05

aelovikov-intel mentioned this pull request Jul 26, 2024

Ill-formed if constexpr branch in vector_operator CTS test for sycl::vec<bool> #896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various smaller bugfixes in tests #870

Various smaller bugfixes in tests #870

fknorr commented Feb 17, 2024 •

edited

Loading

CLAassistant commented Feb 17, 2024 •

edited

Loading

steffenlarsen commented Feb 19, 2024

keryell left a comment

keryell Feb 22, 2024

fknorr Jun 13, 2024

keryell Jun 18, 2024

keryell Feb 22, 2024

fknorr Jun 13, 2024

keryell Feb 22, 2024

AlexeySachkov Feb 23, 2024 •

edited

Loading

keryell Feb 29, 2024

fknorr Jun 13, 2024

keryell Feb 22, 2024

keryell Feb 22, 2024

keryell Feb 22, 2024

fknorr Jun 13, 2024 •

edited

Loading

keryell Feb 22, 2024

fknorr Jun 13, 2024

keryell Feb 22, 2024

fknorr Jun 13, 2024

keryell Feb 22, 2024

fknorr Jun 13, 2024

keryell Feb 22, 2024

fknorr Jun 13, 2024

tomdeakin commented Feb 29, 2024

bader commented Feb 29, 2024

steffenlarsen commented Mar 7, 2024

fknorr commented Jun 13, 2024

keryell left a comment

keryell left a comment

keryell Jun 18, 2024

bader commented Jun 18, 2024

	static sycl::range<2> id_to_range(sycl::id<2> id) {
	static sycl::range<2> to_range(sycl::id<2> id) {

		if (!std::is_same_v<DataT, bool>) {
		static const auto unary_post_operators =

Various smaller bugfixes in tests #870

Various smaller bugfixes in tests #870

Conversation

fknorr commented Feb 17, 2024 • edited Loading

CLAassistant commented Feb 17, 2024 • edited Loading

steffenlarsen commented Feb 19, 2024

keryell left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeySachkov Feb 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fknorr Jun 13, 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

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

Choose a reason for hiding this comment

tomdeakin commented Feb 29, 2024

bader commented Feb 29, 2024

steffenlarsen commented Mar 7, 2024

fknorr commented Jun 13, 2024

keryell left a comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Jun 18, 2024

fknorr commented Feb 17, 2024 •

edited

Loading

CLAassistant commented Feb 17, 2024 •

edited

Loading

AlexeySachkov Feb 23, 2024 •

edited

Loading

fknorr Jun 13, 2024 •

edited

Loading