tests for cl_khr_spirv_queries by bashbaug · Pull Request #2409 · KhronosGroup/OpenCL-CTS

bashbaug · 2025-06-05T05:49:11Z

kpet · 2025-06-06T13:56:42Z

test_conformance/api/spirv_capability_deps.def

@@ -0,0 +1,312 @@
+//


If the SPIRV-Headers become a hard dependency, maybe we should just generate this file at build time?

Done. I initially didn't want to add a build dependency on python, but we already need python to build SPIR-V binaries for the spirv_new test, so perhaps this is OK?

Maybe this test should move to the spriv_new executable instead of api, since it's SPIR-V related? Then the dependency on python will all be in one place?

Or, if we decide we really don't want to bother with generation at build time, we can just drop the last commit.

Maybe this test should move to the spriv_new executable instead of api, since it's SPIR-V related? Then the dependency on python will all be in one place?

I looked at moving this test to spirv_new instead of api and I think this would be relatively straightforward, but there is one side effect of moving the test to spirv_new we should consider: the spirv_new tests only run on a device that supports SPIR-V, as determined by check_spirv_compilation_readiness(). Although it'd debatably be a little silly to do so, we do not currently require a device to support SPIR-V to support cl_khr_spirv_queries, and a device that does not support SPIR-V could return the empty set for all of the new SPIR-V queries and be conformant. Is this a scenario we care about?

If so, we should not move the test to spirv_new. If not, we should document the SPIR-V requirement, and perhaps add a basic "consistency check" to ensure a device advertising support for cl_khr_spirv_queries also supports SPIR-V.

@kpet @karolherbst any strong opinions? Should we keep the test where it is currently, in test_api, or move it to test_spirv_new? I believe this is the last major question to resolve before merging.

If we move the test to test_spirv_new I will add a very API consistency test that runs on all devices. Thinking a bit more, this could take one of two forms:

It could check that if a device supports cl_khr_spirv_queries and does not support SPIR-V that the queries succeed, but are empty. This is basically the current behavior.

Or, if we decide to require SPIR-V support if a device supports cl_khr_spirv_queries, it could check for this coupling.

Since cl_khr_spirv_queries support is now in the upstream headers, we can use the defines from the headers vs. duplicating them in the tests.

karolherbst · 2025-06-10T20:24:43Z

looks good, tested my implementation of cl_khr_spirv_queries against this PR and everything passes.

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

bashbaug · 2025-06-10T21:57:25Z

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

Good catch. I added this, although I don't have a device to test it with, and according to opencl.gpuinfo.org there aren't any devices in the database that support it, either, hrm.

As an aside, the tests are currently (informationally) printing any non-required SPIR-V capabilities that are reported. Should I do something similar for SPIR-V extended instruction sets and SPIR-V extensions also? This might help to find other missing checks like this one... or it could just be noise, once usage of the SPIR-V queries becomes more widespread.

karolherbst · 2025-06-10T22:09:58Z

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

Good catch. I added this, although I don't have a device to test it with, and according to opencl.gpuinfo.org there aren't any devices in the database that support it, either, hrm.

don't have tests here for that either.

As an aside, the tests are currently (informationally) printing any non-required SPIR-V capabilities that are reported. Should I do something similar for SPIR-V extended instruction sets and SPIR-V extensions also? This might help to find other missing checks like this one... or it could just be noise, once usage of the SPIR-V queries becomes more widespread.

I think with this extension in mind it's fine to report more capabilities or extensions than necessary. They can also be part of vendor/ext extensions the tests here isn't aware off, so I'm pretty sure it's gonna cause some noise sooner or later.

kpet · 2025-06-11T08:23:41Z

test_conformance/api/test_spirv_queries.cpp

+    if (version == Version(2, 2))
+    {
+        capabilities.push_back(spv::CapabilityPipeStorage);
+    }
+
+    // Required for OpenCL 2.2, or OpenCL 3.0 devices supporting sub-groups.
+    if (version == Version(2, 2)
+        || (version >= Version(3, 0) && deviceSubGroupsSupport == CL_TRUE))
+    {
+        capabilities.push_back(spv::CapabilitySubgroupDispatch);
+    }


Shouldn't these requirements only apply to devices that support SPIR-V 1.1?

Yes, good catch! Fixed.

I think I'm going to leave the check for SPIR-V 1.1 as-is for now, but after thinking about this a bit more, shouldn't these capability really be supported whenever a device supports SPIR-V 1.1 or newer? The capabilities were added in SPIR-V 1.1, but until they're removed in some future SPIR-V version, I expect they would be available even if a device only supported, say, SPIR-V 1.2.

This might also affect how we document these capabilities in the SPIR-V environment spec. Maybe I should file an issue for this, specifically.

Yeah, it should be 1.1 or newer because support for SPIR V 1.2 would in turn pull 1.1 capability requirements.

I'd like to leave this as-is for now, is possible, in the interest of getting these changes in soon (we're already seeing some "failures" due to the missing changes to compiler_defines_for_extensions). My rationale is:

This probably isn't an issue in practice currently, since most implementations that advertise support for newer versions of SPIR-V (e.g. 1.2 or newer) also advertise support for the older versions of SPIR-V (1.1)

Even if an implementation only advertises support for the newer versions of SPIR-V, we'll just skip a few checks, and the tests won't "fail" unexpectedly.

I'd be happy to file an issue to fix this so it doesn't slip through the cracks.

karolherbst · 2025-06-12T08:14:48Z

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

otherwise test_compiler compiler_defines_for_extensions will fail: "FAIL: Extension cl_khr_spirv_queries is not in the list of approved Khronos extensions!"

This change could go on its own, but I think it's fine adding it here if this PR gets merged soonish.

bashbaug · 2025-06-12T16:20:52Z

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

Fixed.

I really want to get that test fixed; it's kind of silly how we need to modify it every time a new KHR extension gets published.

karolherbst · 2025-06-12T16:31:37Z

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

Fixed.

I really want to get that test fixed; it's kind of silly how we need to modify it every time a new KHR extension gets published.

yeah.. this list really should be generated

While #2409 is under review, could we please add "cl_khr_spirv_queries" to the list of known extensions? This will prevent test "failures" for implementations that support the extension.

kpet · 2025-08-13T07:11:07Z

Merging as discussed in 2025/08/12 teleconference.

tests for cl_khr_spirv_queries

0fb1494

pjaaskel mentioned this pull request Jun 5, 2025

add support for cl_khr_spirv_queries pocl/pocl#1945

Merged

kpet reviewed Jun 6, 2025

View reviewed changes

bashbaug added the focused review label Jun 10, 2025

use defines from headers

3b16ff6

Since cl_khr_spirv_queries support is now in the upstream headers, we can use the defines from the headers vs. duplicating them in the tests.

add checking for cl_khr_spirv_extended_debug_info

caaa0e3

kpet reviewed Jun 11, 2025

View reviewed changes

add missing checks for SPIR-V 1.1 support for some capabilities

dff1fcc

add extension to compiler_defines_for_extensions

72b8521

automatically generate the SPIR-V capabilities defs file

c57c46c

bashbaug mentioned this pull request Jul 10, 2025

July 2025 Release Checklist KhronosGroup/OpenCL-Docs#1409

Closed

28 tasks

karolherbst approved these changes Jul 15, 2025

View reviewed changes

bashbaug mentioned this pull request Jul 16, 2025

[SYCL][Doc] Add proposed sycl_ext_oneapi_spirv_queries spec intel/llvm#19435

Merged

Merge branch 'main' into cl_khr_spirv_queries

c8594ee

bashbaug mentioned this pull request Jul 30, 2025

add cl_khr_spirv_queries to list of known extensions #2463

Merged

ahesham-arm approved these changes Aug 12, 2025

View reviewed changes

kpet approved these changes Aug 13, 2025

View reviewed changes

kpet merged commit 555b7cd into KhronosGroup:main Aug 13, 2025
7 checks passed

joselopeqti mentioned this pull request Sep 12, 2025

Remove hard dependency on SPIR-V Headers #2521

Closed

Conversation

bashbaug commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karolherbst commented Jun 10, 2025

Uh oh!

bashbaug commented Jun 10, 2025

Uh oh!

karolherbst commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karolherbst commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bashbaug commented Jun 12, 2025

Uh oh!

karolherbst commented Jun 12, 2025

Uh oh!

kpet commented Aug 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karolherbst commented Jun 12, 2025 •

edited

Loading