Implement tests for `SYCL_KHR_DEFAULT_CONTEXT` extension #960

0x12CC · 2024-10-07T02:58:43Z

Implement the tests from #944.

Signed-off-by: Michael Aziz <[email protected]>

Spec: KhronosGroup/SYCL-Docs#624 sycl-cts: KhronosGroup/SYCL-CTS#960 This patch is intended to be merged only when there are sycl-cts tests and the ratified khr extension.

Signed-off-by: Michael Aziz <[email protected]>

psalz

Since this is the first KHR extension test, we should think about how we want to approach this in general. Currently you have both a CMake option to enable/disable compilation of tests for the extension, as well as compile-time guards that check whether the extension is available. I'm wondering whether we need both.

If we keep the former (which I think is a good idea, at least for now -- we'll see whether it scales), I'd argue we can get rid of the #ifdef SYCL_KHR_DEFAULT_CONTEXT. If someone wants to test the extension, they'd likely want to know that it isn't available during compilation, not at runtime.

tests/extension/khr_default_context/default_context.cpp

gmlueck · 2024-10-14T13:08:56Z

If we keep the former (which I think is a good idea, at least for now -- we'll see whether it scales), I'd argue we can get rid of the #ifdef SYCL_KHR_DEFAULT_CONTEXT. If someone wants to test the extension, they'd likely want to know that it isn't available during compilation, not at runtime.

I think this makes sense. If the CTS is run with SYCL_CTS_ENABLE_EXT_KHR_DEFAULT_CONTEXT_TESTS, then it seems like the test suite should fail if the implementation does not define SYCL_KHR_DEFAULT_CONTEXT. Otherwise, an implementation could pass the CTS for this extension even without implementing it.

CMakeLists.txt

Signed-off-by: Michael Aziz <[email protected]>

0x12CC · 2024-10-16T02:44:57Z

@psalz, @gmlueck, thanks for taking a look at this!

Since this is the first KHR extension test, we should think about how we want to approach this in general. Currently you have both a CMake option to enable/disable compilation of tests for the extension, as well as compile-time guards that check whether the extension is available. I'm wondering whether we need both.

I think we need both. I did this to make the new extension test consistent with the other extension tests in the CTS. We have a similar CMake option for SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS, and tests use SKIP if a feature test macro is not defined.

If we keep the former (which I think is a good idea, at least for now -- we'll see whether it scales), I'd argue we can get rid of the #ifdef SYCL_KHR_DEFAULT_CONTEXT. If someone wants to test the extension, they'd likely want to know that it isn't available during compilation, not at runtime.

If I get rid of this macro, someone that wants to test an implementation that implements most features and not this one would need to enumerate all of the relevant CMake options. They wouldn't be able to use the SYCL_CTS_ENABLE_KHR_TESTS option since this test would fail to compile.

I think this makes sense. If the CTS is run with SYCL_CTS_ENABLE_EXT_KHR_DEFAULT_CONTEXT_TESTS, then it seems like the test suite should fail if the implementation does not define SYCL_KHR_DEFAULT_CONTEXT. Otherwise, an implementation could pass the CTS for this extension even without implementing it.

Catch2 distinguishes passing test cases from skipped ones. Here's the output for this test when the macro is not defined:

SYCL-CTS/tests/extension/khr_default_context/default_context.cpp:72: SKIPPED:
explicitly with message:
  SYCL_KHR_DEFAULT_CONTEXT is not defined

================================================================================
test cases: 1 | 1 skipped
assertions: - none -

If you both still agree that the SYCL_KHR_DEFAULT_CONTEXT macro guard is not needed then I'll remove it from the test. Also, if we do choose to remove it, should we remove the macros from the other extension tests (in a different PR)?

psalz · 2024-10-16T09:06:28Z

If I get rid of this macro, someone that wants to test an implementation that implements most features and not this one would need to enumerate all of the relevant CMake options. They wouldn't be able to use the SYCL_CTS_ENABLE_KHR_TESTS option since this test would fail to compile.

Is that really a common use-case though? Realistically, I would assume that it's mostly going to be Intel that runs the CTS for the time being. If it does get out of hand, we could always provide CMake presets for the supported implementations.

Catch2 distinguishes passing test cases from skipped ones.

True, I'm just not sure whether having that skip message is worth sacrificing legibility of the code by either grouping the tests into guarded functions instead of using individual TEST_CASEs (like you did in this PR), or by littering #ifdefs all over the place.

psalz · 2024-10-16T09:09:19Z

Of course we could also get fancy and do a configuration-time check (try_compile) to see whether an extension is available, and if not, compile a proxy translation unit that only prints the skipped message.

gmlueck · 2024-10-16T12:23:35Z

Catch2 distinguishes passing test cases from skipped ones. Here's the output for this test when the macro is not defined:
SYCL-CTS/tests/extension/khr_default_context/default_context.cpp:72: SKIPPED:
explicitly with message:
  SYCL_KHR_DEFAULT_CONTEXT is not defined

================================================================================
test cases: 1 | 1 skipped
assertions: - none -

This requires a human to examine the test output, though. I think it would be better for the test to fail (return a non-zero exit status) if the macro isn't defined. This will make the tests easier to run in a CI system, which just tests the exit status.

I think it would also be good for the Khronos workgroup when a vendor submits a conformance report. The WG won't want to look through the test output to see if there are SKIPPED messages. It will be easier for the WG to just rely on the fact that the CTS passed when determining a vendor's conformance.

If you both still agree that the SYCL_KHR_DEFAULT_CONTEXT macro guard is not needed then I'll remove it from the test.

I think we should go a step further. We should remove the macro guard, and also add a test that explicitly tests that the macro is defined. Part of an extension's API is that the macro is supposed to be set, so the CTS should check this.

Also, if we do choose to remove it, should we remove the macros from the other extension tests (in a different PR)?

The other extensions are all "oneapi" extensions, right? Putting my Khronos hat on ... these extensions exist in the Khronos CTS as a convenience to DPC++, so DPC++ can decide what works best for them.

Putting my DPC++ hat on ... I think this probably does make sense. We've had situations in the past where we forgot to define the macro for an extension. Restructuring the tests like this would catch these errors. Of course, we'd need to have some cmake option to control when the oneapi tests are run. Do we have something like SYCL_CTS_ENABLE_ONEAPI_TESTS already?

If I get rid of this macro, someone that wants to test an implementation that implements most features and not this one would need to enumerate all of the relevant CMake options. They wouldn't be able to use the SYCL_CTS_ENABLE_KHR_TESTS option since this test would fail to compile.

One more comment about this. When vendors submit official conformance results, I think we do not want them to use SYCL_CTS_ENABLE_KHR_TESTS because it's not obvious which extensions are being tested. Wouldn't we want official runs to pass a list of SYCL_CTS_ENABLE_KHR_DEFAULT_CONTEXT_TESTS, ... options, so we know exactly which extensions the vendor is claiming conformance?

Signed-off-by: Michael Aziz <[email protected]>

0x12CC · 2024-10-17T12:30:57Z

Is that really a common use-case though? Realistically, I would assume that it's mostly going to be Intel that runs the CTS for the time being. If it does get out of hand, we could always provide CMake presets for the supported implementations.

I don't think it's common and we likely don't need to provide any presets. I just wanted to make sure I understood the difference with your suggested approach.

This requires a human to examine the test output, though. I think it would be better for the test to fail (return a non-zero exit status) if the macro isn't defined. This will make the tests easier to run in a CI system, which just tests the exit status.

I think it would also be good for the Khronos workgroup when a vendor submits a conformance report. The WG won't want to look through the test output to see if there are SKIPPED messages. It will be easier for the WG to just rely on the fact that the CTS passed when determining a vendor's conformance.

I think this makes sense. The updated test case will fail due to a static_assert if the macro is not defined. This is slightly different than your suggestion to have the test fail at runtime but should not require the Khronos workgroup to look through any test outputs.

I think we should go a step further. We should remove the macro guard, and also add a test that explicitly tests that the macro is defined. Part of an extension's API is that the macro is supposed to be set, so the CTS should check this.

I've added a test case for this using static_assert since @psalz suggested we catch the issue during compilation rather than runtime. I can change this test case to fail at runtime but would need to use the macro guard on the other test cases to make sure they don't cause other compilation errors. Compilation without the macro shows the following error:

SYCL-CTS/tests/extension/khr_default_context/default_context.cpp:29:17: error: static assertion failed: SYCL_KHR_DEFAULT_CONTEXT is not defined
   29 |   static_assert(false, "SYCL_KHR_DEFAULT_CONTEXT is not defined");

The other extensions are all "oneapi" extensions, right? Putting my Khronos hat on ... these extensions exist in the Khronos CTS as a convenience to DPC++, so DPC++ can decide what works best for them.

Putting my DPC++ hat on ... I think this probably does make sense. We've had situations in the past where we forgot to define the macro for an extension. Restructuring the tests like this would catch these errors. Of course, we'd need to have some cmake option to control when the oneapi tests are run. Do we have something like SYCL_CTS_ENABLE_ONEAPI_TESTS already?

Yes, all other extensions are oneAPI extensions. We have the SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS option to enable all of the extension tests. We also have one option for each extension but our regular CTS builds use SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS.

One more comment about this. When vendors submit official conformance results, I think we do not want them to use SYCL_CTS_ENABLE_KHR_TESTS because it's not obvious which extensions are being tested. Wouldn't we want official runs to pass a list of SYCL_CTS_ENABLE_KHR_DEFAULT_CONTEXT_TESTS, ... options, so we know exactly which extensions the vendor is claiming conformance?

I don't have any experience reviewing conformance results but your suggestion seems reasonable. Conformance submissions using the SYCL_CTS_ENABLE_KHR_TESTS option may be confusing since new extensions can be added to this option at a later date.

psalz · 2024-10-17T12:44:33Z

tests/extension/khr_default_context/default_context.cpp

+    "the default context extension defines the SYCL_KHR_DEFAULT_CONTEXT macro",
+    "[khr_default_context]") {
+#ifndef SYCL_KHR_DEFAULT_CONTEXT
+  static_assert(false, "SYCL_KHR_DEFAULT_CONTEXT is not defined");


I think this is a good solution!

psalz · 2024-10-17T12:45:06Z

tests/extension/khr_default_context/default_context.cpp

+  sycl::context defaultContext = sycl::platform{}.khr_get_default_context();
+
+  // Check that a default-constructed context is not the default context.
+  CHECK(syclContext != defaultContext);


Minor: This could be pulled out into it's own test case IMO.

gmlueck

I think this looks great!

If you are on a roll with tests for default context behavior, we also need tests for #943. Note that those tests are for the core spec, so they are not tied to the extension.

tomdeakin · 2024-10-17T15:25:27Z

Thanks! Waiting for further feedback from AdaptiveCpp.

tomdeakin · 2024-11-14T17:00:15Z

WG approved to merge.

Implement tests for sycl_ext_oneapi_default_context extension

8866c74

Signed-off-by: Michael Aziz <[email protected]>

gmlueck mentioned this pull request Oct 7, 2024

Add a test plan for the sycl_ext_oneapi_default_context extension #959

Closed

KornevNikita mentioned this pull request Oct 9, 2024

[SYCL] Support sycl_khr_default_context intel/llvm#15645

Draft

Change tests to test KHR extension

313a284

Signed-off-by: Michael Aziz <[email protected]>

0x12CC changed the title ~~Implement tests for sycl_ext_oneapi_default_context extension~~ Implement tests for SYCL_KHR_DEFAULT_CONTEXT extension Oct 12, 2024

Run clang-format

cfbd17a

Signed-off-by: Michael Aziz <[email protected]>

0x12CC marked this pull request as ready for review October 12, 2024 21:13

0x12CC requested a review from a team as a code owner October 12, 2024 21:13

psalz reviewed Oct 14, 2024

View reviewed changes

tests/extension/khr_default_context/default_context.cpp Outdated Show resolved Hide resolved

tests/extension/khr_default_context/default_context.cpp Outdated Show resolved Hide resolved

gmlueck reviewed Oct 14, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

0x12CC added 2 commits October 15, 2024 19:20

Remove EXT prefix and fix test tag

7325477

Signed-off-by: Michael Aziz <[email protected]>

Remove EXT prefix

cffbf6e

Signed-off-by: Michael Aziz <[email protected]>

Remove SKIP macro

8f39ce4

Signed-off-by: Michael Aziz <[email protected]>

0x12CC requested review from psalz and gmlueck October 17, 2024 12:31

psalz approved these changes Oct 17, 2024

View reviewed changes

gmlueck approved these changes Oct 17, 2024

View reviewed changes

bader added 2 commits November 8, 2024 11:25

Merge branch 'main' into default_context_tests

0f660ea

Merge branch 'main' into default_context_tests

16a3bcb

illuhad approved these changes Nov 14, 2024

View reviewed changes

tomdeakin merged commit ead7474 into KhronosGroup:main Nov 14, 2024
8 checks passed

0x12CC deleted the default_context_tests branch November 14, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tests for `SYCL_KHR_DEFAULT_CONTEXT` extension #960

Implement tests for `SYCL_KHR_DEFAULT_CONTEXT` extension #960

0x12CC commented Oct 7, 2024 •

edited

Loading

psalz left a comment

gmlueck commented Oct 14, 2024

0x12CC commented Oct 16, 2024

psalz commented Oct 16, 2024

psalz commented Oct 16, 2024

gmlueck commented Oct 16, 2024

0x12CC commented Oct 17, 2024

psalz Oct 17, 2024

psalz Oct 17, 2024

gmlueck left a comment

tomdeakin commented Oct 17, 2024

tomdeakin commented Nov 14, 2024

Implement tests for SYCL_KHR_DEFAULT_CONTEXT extension #960

Implement tests for SYCL_KHR_DEFAULT_CONTEXT extension #960

Conversation

0x12CC commented Oct 7, 2024 • edited Loading

psalz left a comment

Choose a reason for hiding this comment

gmlueck commented Oct 14, 2024

0x12CC commented Oct 16, 2024

psalz commented Oct 16, 2024

psalz commented Oct 16, 2024

gmlueck commented Oct 16, 2024

0x12CC commented Oct 17, 2024

psalz Oct 17, 2024

Choose a reason for hiding this comment

psalz Oct 17, 2024

Choose a reason for hiding this comment

gmlueck left a comment

Choose a reason for hiding this comment

tomdeakin commented Oct 17, 2024

tomdeakin commented Nov 14, 2024

Implement tests for `SYCL_KHR_DEFAULT_CONTEXT` extension #960

Implement tests for `SYCL_KHR_DEFAULT_CONTEXT` extension #960

0x12CC commented Oct 7, 2024 •

edited

Loading