Skip to content

[SYCL][Doc] Add proposed sycl_ext_oneapi_spirv_queries spec #19435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 11, 2025

Conversation

bashbaug
Copy link
Contributor

This is a proposed SYCL extension to query the SPIR-V extended instruction sets, SPIR-V extensions, and SPIR-V capabilities supported by a SYCL device. It essentially provides the same functionality as the OpenCL cl_khr_spirv_queries extension via SYCL.

@bashbaug bashbaug requested a review from a team as a code owner July 14, 2025 18:34
* Added device aspects for SPIR-V and SPIR-V queries support.
* Added queries for SPIR-V version support.
* Added device get_info queries that return sets of supported features.
* Many formatting and cosmetic improvements.

Signed-off-by: Ben Ashbaugh <[email protected]>

struct spirv_version {
uint16_t major;
uint16_t minor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we use these fixed-width types too much in the SYCL spec. Technically, a C++ implementation isn't even required to provide them. I think it would be better to either rely on bitfields like:

struct spirv_version {
  unsigned major:16;
  unsigned minor:16;
};

Or just use a fundamental type that is guaranteed to be at least 16 bits:

struct spirv_version {
  unsigned short major;
  unsigned short minor;
};

The same for "capability", where I think we could use unsigned long instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "at least 16 bits" part is surprising, though, and may lead to potentially weird behavior. If a host defines "short" as 32-bit, then the device compiler has to be configured to use that. With the fixed types it's much clearer that things are intended to be 16 bits and should be 16 bits everywhere.

For a type like this it might not matter so much, because spirv_version probably won't cross the host-device boundary. I'm just mentioning this because I would personally like to see us continue to use fixed-width types in other extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I had a bitfield implementation and originally and switched to a fixed-width type instead.

I've gone back to a bitfield for now, but I'm fine either way, just let me know what you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I'd strongly prefer not to switch "capability" to unsigned long though, since the size of an unsigned long is not fixed on some platforms. I think this is OK since uint32_t is used in many other places in the SYCL spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to die on this hill, but I don't really understand either of your concerns.

For SYCL implementations that are multi-pass, SYCL requires both the host and the device compiler passes to define the types identically. Otherwise, we would have all sorts of problems passing data between the host and device code. Therefore, there would be no problem passing spirv_version across the boundary even if it were defined using fundamental types that do not have a guaranteed width.

Regarding unsigned long being defined differently in different environments -- this is definitely the case. Windows and Linux have traditionally defined this type differently. Pedantically, a compiler can choose the width for most of the fundamental C/C++ types, so many of them are not necessarily the same on every compiler. It's not clear to me why this is a reason not to use these types in SYCL APIs (or in any other API for that matter). The fundamental types in C/C++ do not have a guaranteed width, and people have been writing code just fine despite this fact.

I do admit that SYCL is full of fixed-width types now. I just feel like we use them too often, and in many cases we should just use a fundamental type instead. Maybe that's just a personal stylistic preference, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting anything across Windows/Linux is a pain for reasons beyond just types, so I agree that people will expect weirdness and be happy to handle it in that case.

The reason I don't like the variable-width types in SYCL specifically is that it's supposed to be portable, and the compiler might end up making surprising decisions that go against a developer's intent. Theoretically, a developer could write and tune a GPU kernel on a specific combination of OS and device (say, Linux + an Intel GPU), move to another system that they think looks very similar (same OS, same GPU, same compiler, but different host CPU) and the size of all their types could change. Types suddenly getting bigger could impact a kernel's performance (e.g., because it now needs more registers) or even prevent a kernel from working (e.g., because it now needs more work-group local memory than the device has).

FWIW, this isn't a problem in OpenCL because the types are not defined in terms of minimum width (see here): in OpenCL C, short always means "a 16 bit-integer", and cl_short exists to allow a stable API to be exposed to host code. SYCL using the fixed-width types solves the same problem.

* switched from a std::set to a set::vector
* switched to struct bitfields vs. uint16_t
* throw an exception if the device does not support SPIR-V queries

Signed-off-by: Ben Ashbaugh <[email protected]>
@gmlueck
Copy link
Contributor

gmlueck commented Aug 11, 2025

@intel/llvm-gatekeepers I think this is ready to merge.

@uditagarwal97 uditagarwal97 merged commit b752163 into intel:sycl Aug 11, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants