Tests for `[[device_has]]` attribute from optional kernel features #287

andreyromanof · 2022-02-17T02:12:47Z

This PR provides tests for optional kernel features.
Test decorates kernel or function invoked by the kernel with the attribute [[sycl::device_has(aspect)]] and expected exception if a device doesn't support aspect, otherwise expect no throw.

Depends on:

Disable _Pragma to work around Clang 8 bug #296

* Added cmake for deivce_has tests * Added common code for device_has tests * Added tests for device_has attribute * Tests added to filter for computecpp and hipsycl

ProGTX · 2022-02-17T09:15:51Z

ci/computecpp.filter

@@ -28,3 +28,4 @@ usm
 vector_api
 vector_constructors
 vector_swizzles
+optional_kernel_features


Same naming question as in #285

tests/optional_kernel_features/kernel_features_separate_unit.cpp

According to spec SYCL_EXTERNAL is optional and can be unsupported by some implemetations. If implementation doesn't define SYCL_EXTERNAL then tests with external functions will be disabled

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp

bader

@andreyromanof, could you extend the description of the PR with the information about what prevent the compilation of this test with computcpp/dpcpp/hipsycl?

andreyromanof · 2022-02-25T02:11:01Z

@andreyromanof, could you extend the description of the PR with the information about what prevent the compilation of this test with computcpp/dpcpp/hipsycl?

Done

keryell

You could remove some useless ()

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp

tests/optional_kernel_features/kernel_features_separate_unit.cpp

Also changed `device_has()` to `get_device().has()`

bader

Build fails on computecpp due to linkage error, which I was unable to solve.

@andreyromanof, the link points to the log for a different PR. According to @psalz, there are at least two issues with compiling this PR with computecpp:

tests use unnamed lambda
tests use "device_has"

I expected you to list both in the description.

I suggest we remove optional_kernel_features from all the filters and disable specific test cases with the macro introduced by #293.

andreyromanof · 2022-02-25T09:07:21Z

I suggest we remove optional_kernel_features from all the filters and disable specific test cases with the macro introduced by #293.

That makes sense. I'm going to patch this test and tests from other PRs.

…into optional_kernel_features/device_has_exception

andreyromanof · 2022-02-28T02:21:06Z

I added compile time disable of the test and removed this test from the filter.
I also noticed, that for some reason I should pass implementation names in strict order. For example

DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, DPCPP, hipSYCL) build fails due strange errors
DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, hipSYCL, DPCPP) build finishes with no errors

@psalz could you please verify is this a bug or not.

andreyromanof · 2022-02-28T02:29:15Z

Build still fails on hipSYCL because there is no sycl::errc definition.

psalz · 2022-02-28T08:53:14Z

I added compile time disable of the test and removed this test from the filter. I also noticed, that for some reason I should pass implementation names in strict order. For example
* `DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, DPCPP, hipSYCL)` build fails due strange errors

* `DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, hipSYCL, DPCPP)` build finishes with no errors
@psalz could you please verify is this a bug or not.

Yes same bug as in #289, fixed in #295.

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp

bader

Will this work?

ci/hipsycl.filter

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp

…into optional_kernel_features/device_has_exception

bader · 2022-03-01T09:44:43Z

@andreyromanof, does merging #299 and #296 fix pre-commit?

andreyromanof · 2022-03-01T09:53:23Z

Can't say for sure.
#296 should fix build for computecpp which I can't test locally. Also, Philip mentioned a bug with linkage even after with fix. So probably we will need to update computecpp version too.
#299 Should fix ci build for dpcpp.

…into optional_kernel_features/device_has_exception

keryell · 2022-03-04T18:39:19Z

tests/optional_kernel_features/kernel_features_common.h

+/**
+ * @brief Macro for generating code that will use TYPE
+ */
+#define USE_FEATURE(TYPE)       \


Not clear why you are using a macro instead of a generic function or lambda.

This macro should help to test cases, when user try to use feature function directly in kernel body, wihout functions or lambda. For example:

// using feature directly from kernel queue.submit([&](sycl::handler &cgh) { cgh.single_task([] { dobule d = 0; d += 42; }); }); // using feature in function call queue.submit([&](sycl::handler &cgh) { cgh.single_task([] { some_function_use_double(); }); });

The goal was test separately cases with functions, that use some feature and test cases, when feature is used by kernel directly.

keryell · 2022-03-04T18:43:45Z

tests/optional_kernel_features/kernel_features_common.h

+      }                                                                        \
+      CHECK(is_exception_thrown == IS_EXCEPTION_EXPECTED);                     \
+      check_async_exception(QUEUE, false);                                     \
+    }                                                                          \


Why such a huge macro instead of a function/class/lambda? Is this to pass the attribute with the preprocessor?
Also a lot of copy-paste inside the macro itself.

I used macro because in some cases kernel should be sent as a submission call (without storing it in separate lambda or class functor for example). To achieve that I wrote this huge macro, that should paste code with submission call.

This macro also refactored in this commit: 2fb2d5e

keryell · 2022-03-04T18:45:59Z

tests/optional_kernel_features/kernel_features_common.h

+      }
+    }
+    CHECK(is_exception_thrown == is_exception_expected);
+    check_async_exception(queue, false);


I have the feeling to mostly see 3 times the same code.
Factorize in a function/lambda called 3 times and taking a lambda for executing the different code?

You're right, it looks like I can optimize these places. I tried to refactor these functions in this commit: 2fb2d5e

keryell · 2022-03-04T18:46:49Z

tests/optional_kernel_features/kernel_features_common.h

+      }
+    }
+    CHECK(is_exception_thrown == is_exception_expected);
+    check_async_exception(queue, false);


Idem: mostly 3 copy-pastes.

…into optional_kernel_features/device_has_exception

Change try/catch blocks to CHECK_THROW_MATCHES and CHECK_NOTHROW Moved exception check to separate function

andreyromanof · 2022-03-05T06:45:08Z

@bader Right now computecpp build fails, because there are using unnamed kernels in the code. Spec says that it's allowed. Should we use a workaround with the named kernel to avoid this build issue, or these tests can just wait in the filter(disabled by macro DISABLED_FOR_TEMPLATE_TEST_CASE_SIG) until feature with automatic kernel names will be implemented in computecpp? Please note, that unnamed kernels are also used in these PRs: #292, #294, #289.

Also, computecpp will not compile even with named kernels due to a linkage error that looks like an implementation bug. (This error occurs in other PR where named kernels are used. Link to linkage error in other PR)

andreyromanof · 2022-03-05T07:51:47Z

@psalz I don't understand why code in kernel_features_device_has_exceptions.cpp still compiling on computecpp, despite the fact that I disable implementation in every test with DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, hipSYCL). Can you, please, suggest to me what I doing wrong? :)

psalz · 2022-03-07T12:15:25Z

@psalz I don't understand why code in kernel_features_device_has_exceptions.cpp still compiling on computecpp, despite the fact that I disable implementation in every test with DISABLED_FOR_TEMPLATE_TEST_CASE_SIG(ComputeCpp, hipSYCL). Can you, please, suggest to me what I doing wrong? :)

Fixed in #303.

…into optional_kernel_features/device_has_exception

andreyromanof · 2022-03-09T04:53:32Z

Fixed in #303.

Thanks.

Early pulldown for `id` tests fix from KhronosGroup#301

Tests for [[device_has]] attribute from optional kernel features

5cb9077

* Added cmake for deivce_has tests * Added common code for device_has tests * Added tests for device_has attribute * Tests added to filter for computecpp and hipsycl

andreyromanof force-pushed the optional_kernel_features/device_has_exception branch from a50e287 to 5cb9077 Compare February 17, 2022 02:20

andreyromanof marked this pull request as ready for review February 17, 2022 07:15

andreyromanof requested review from ProGTX, bader, illuhad and keryell as code owners February 17, 2022 07:15

Sort tests in filter

acc4949

ProGTX approved these changes Feb 17, 2022

View reviewed changes

Romanov AndreyX added 4 commits February 18, 2022 10:17

Add CMakeLists to optional features

78a23b2

Remove tests with SYCL_EXTERNAL if undefined

3fff1f8

According to spec SYCL_EXTERNAL is optional and can be unsupported by some implemetations. If implementation doesn't define SYCL_EXTERNAL then tests with external functions will be disabled

Add optional_kernel_features to filter for dpcpp

9df130e

Add comment to cts_async_handler

b90a11e

andreyromanof mentioned this pull request Feb 21, 2022

Tests for [[reqd_sub_group_size]] attribute from optional kernel features #289

Merged

kopylovanat mentioned this pull request Feb 24, 2022

Add tests for runtime exception if device doesn't support required work group size #292

Merged

bader reviewed Feb 24, 2022

View reviewed changes

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp Outdated Show resolved Hide resolved

bader reviewed Feb 24, 2022

View reviewed changes

Remove duplicate of the test tag from the test name

8736252

andreyromanof mentioned this pull request Feb 25, 2022

Test for speculative compilation of kernels that use supported and unsupported features #294

Merged

keryell reviewed Feb 25, 2022

View reviewed changes

bader mentioned this pull request Feb 25, 2022

[SYCL] Make queue::device_has() private intel/llvm#5670

Merged

Remove empty brackets from lambdas with no arg

559de67

Also changed `device_has()` to `get_device().has()`

bader requested changes Feb 25, 2022

View reviewed changes

Romanov AndreyX added 2 commits February 25, 2022 16:07

Merge branch 'SYCL-2020' of https://github.com/KhronosGroup/SYCL-CTS …

dbddb89

…into optional_kernel_features/device_has_exception

Add runtime disable of the device_has test for all compilers

e456183

Add device_has test to filter for hipSYCL

5c6a8a1

andreyromanof requested a review from keryell February 28, 2022 02:42

Revert "Remove empty brackets from lambdas with no arg"

5d48584

bader reviewed Feb 28, 2022

View reviewed changes

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp Outdated Show resolved Hide resolved

bader reviewed Feb 28, 2022

View reviewed changes

ci/hipsycl.filter Outdated Show resolved Hide resolved

tests/optional_kernel_features/kernel_features_device_has_exceptions.cpp Show resolved Hide resolved

Romanov AndreyX added 2 commits March 1, 2022 09:12

Merge branch 'SYCL-2020' of https://github.com/KhronosGroup/SYCL-CTS …

fe52c59

…into optional_kernel_features/device_has_exception

Split section into separate test cases

e297e1e

andreyromanof mentioned this pull request Mar 1, 2022

Bump dpcpp version in ci #298

Closed

Romanov AndreyX added 2 commits March 1, 2022 10:40

Workaround for hipsycl compilation

01f810d

Move hipsycl workaround to common

85c2713

bader mentioned this pull request Mar 1, 2022

Bump dpcpp version in ci #299

Merged

bader mentioned this pull request Mar 3, 2022

Disable _Pragma to work around Clang 8 bug #296

Merged

Merge branch 'SYCL-2020' of https://github.com/KhronosGroup/SYCL-CTS …

112f25f

…into optional_kernel_features/device_has_exception

keryell reviewed Mar 4, 2022

View reviewed changes

Romanov AndreyX added 2 commits March 5, 2022 09:46

Merge branch 'SYCL-2020' of https://github.com/KhronosGroup/SYCL-CTS …

6f61ddf

…into optional_kernel_features/device_has_exception

Refactoring of kernel_features_common

2fb2d5e

Change try/catch blocks to CHECK_THROW_MATCHES and CHECK_NOTHROW Moved exception check to separate function

Romanov AndreyX added 3 commits March 5, 2022 14:17

Remove computecpp from sycl_enums

572a4f7

Return computecpp filter to sycl_enums

b96566d

Disable common for computecpp

53d40c7

Merge branch 'SYCL-2020' of https://github.com/KhronosGroup/SYCL-CTS …

d75ce07

…into optional_kernel_features/device_has_exception

bader approved these changes Mar 9, 2022

View reviewed changes

bader merged commit 2ba9972 into KhronosGroup:SYCL-2020 Mar 9, 2022

andreyromanof deleted the optional_kernel_features/device_has_exception branch April 7, 2022 08:02

steffenlarsen pushed a commit to steffenlarsen/SYCL-CTS that referenced this pull request Dec 6, 2022

Merge pull request KhronosGroup#287 from yuriykoch/pulldown/PR301

1a493ed

Early pulldown for `id` tests fix from KhronosGroup#301

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for `[[device_has]]` attribute from optional kernel features #287

Tests for `[[device_has]]` attribute from optional kernel features #287

andreyromanof commented Feb 17, 2022 •

edited

ProGTX Feb 17, 2022

bader left a comment

andreyromanof commented Feb 25, 2022

keryell left a comment

bader left a comment

andreyromanof commented Feb 25, 2022

andreyromanof commented Feb 28, 2022

andreyromanof commented Feb 28, 2022 •

edited

psalz commented Feb 28, 2022

bader left a comment

bader commented Mar 1, 2022

andreyromanof commented Mar 1, 2022

keryell Mar 4, 2022

andreyromanof Mar 5, 2022

keryell Mar 4, 2022

andreyromanof Mar 5, 2022 •

edited

keryell Mar 4, 2022

andreyromanof Mar 5, 2022 •

edited

keryell Mar 4, 2022

andreyromanof commented Mar 5, 2022 •

edited

andreyromanof commented Mar 5, 2022 •

edited

psalz commented Mar 7, 2022

andreyromanof commented Mar 9, 2022

Tests for [[device_has]] attribute from optional kernel features #287

Tests for [[device_has]] attribute from optional kernel features #287

Conversation

andreyromanof commented Feb 17, 2022 • edited

ProGTX Feb 17, 2022

Choose a reason for hiding this comment

bader left a comment

Choose a reason for hiding this comment

andreyromanof commented Feb 25, 2022

keryell left a comment

Choose a reason for hiding this comment

bader left a comment

Choose a reason for hiding this comment

andreyromanof commented Feb 25, 2022

andreyromanof commented Feb 28, 2022

andreyromanof commented Feb 28, 2022 • edited

psalz commented Feb 28, 2022

bader left a comment

Choose a reason for hiding this comment

bader commented Mar 1, 2022

andreyromanof commented Mar 1, 2022

keryell Mar 4, 2022

Choose a reason for hiding this comment

andreyromanof Mar 5, 2022

Choose a reason for hiding this comment

keryell Mar 4, 2022

Choose a reason for hiding this comment

andreyromanof Mar 5, 2022 • edited

Choose a reason for hiding this comment

keryell Mar 4, 2022

Choose a reason for hiding this comment

andreyromanof Mar 5, 2022 • edited

Choose a reason for hiding this comment

keryell Mar 4, 2022

Choose a reason for hiding this comment

andreyromanof commented Mar 5, 2022 • edited

andreyromanof commented Mar 5, 2022 • edited

psalz commented Mar 7, 2022

andreyromanof commented Mar 9, 2022

Tests for `[[device_has]]` attribute from optional kernel features #287

Tests for `[[device_has]]` attribute from optional kernel features #287

andreyromanof commented Feb 17, 2022 •

edited

andreyromanof commented Feb 28, 2022 •

edited

andreyromanof Mar 5, 2022 •

edited

andreyromanof Mar 5, 2022 •

edited

andreyromanof commented Mar 5, 2022 •

edited

andreyromanof commented Mar 5, 2022 •

edited