Skip to content
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

What extensions need to be reported through CL_PLATFORM_EXTENSIONS #1287

Open
karolherbst opened this issue Dec 2, 2024 · 7 comments
Open

Comments

@karolherbst
Copy link
Contributor

One sentence in the spec which I'm not entirely sure how to parse is "Each extension that is supported by all devices associated with this platform must be reported here.".

As of now in rusticl/mesa we only report extensions which don't rely on any hardware support and only those we promise to support on every possibly supported device.

However one could also read this as "extensions supported by all devices by this platform need to be listed there". "all devices" as in all the currently available and listed ones.

We are probably one of the few OpenCL implementations where distinguishing between those two interpretations might even matter and I was wondering if it makes sense to clarify this or what the actual intention here was.

@bashbaug
Copy link
Contributor

bashbaug commented Dec 5, 2024

I remember being confused about this also.

I read "Each extension that is supported by all devices associated with this platform must be reported here" to mean:

  • If an extension is listed in CL_PLATFORM_EXTENSIONS then it also must be listed in CL_DEVICE_EXTENSIONS for each of the devices enumerated by clGetDeviceIDs for that platform.
  • In other words, CL_PLATFORM_EXTENSIONS contains the intersection of the set of extensions supported by all of the devices in the platform.

There's a CTS that kind of checks for this. It's a little weird though, in that it only tracks a specific set of extensions. IMHO this test should be expanded so it checks for all of the extensions reported in CL_PLATFORM_EXTENSIONS generally, and not only a specific set.

https://github.com/KhronosGroup/OpenCL-CTS/blob/main/test_conformance/api/test_platform.cpp#L24

@karolherbst
Copy link
Contributor Author

I think this test looks entirely broken, especially as extensions and extensionsSupported are out of sync, probably because of the // "cl_APPLE_SetMemObjectDestructor", line. But extensionsSupported is all false anyway. And reading the code I don't think it could ever reach the vlog_error("Platform supports extension \"%s\" but device does not\n", check.

But yeah, I think it makes sense to clean this up and to clarify what we want to test here.

I can certainly change the behavior in rusticl to check all devices and report all common extensions, if that's what's expected here anyway.

@karolherbst
Copy link
Contributor Author

I think my point here is, that we don't know how all the other implementations are behaving and if they'd even pass once that test gets fixed.

Should probably bring it up on the next WG meeting and see what others say.

@bashbaug
Copy link
Contributor

bashbaug commented Dec 8, 2024

KhronosGroup/OpenCL-CTS#2172 updates the test so it works the way I expect it should work.

It's passing on most of the implementations I've tested, though not all: one implementation is returning cl_khr_icd for the platform but not for the device.

@bashbaug
Copy link
Contributor

Discussed in the December 10th teleconference. Our current thinking is:

  1. If an extension is supported by all of the devices in the platform then the extension must be reported through CL_PLATFORM_EXTENSIONS.
  2. There are some additional extensions that may be supported through CL_PLATFORM_EXTENSIONS that do not make sense to report by a device, such as cl_khr_icd. This is uncommon, but it does happen. This means that CL_PLATFORM_EXTENSIONS may include additional extensions beyond the intersection of all device extensions.

Next steps are:

  1. We need to determine which extensions are purely platform extensions that will be reported by CL_PLATFORM_EXTENSIONS but not CL_DEVICE_EXTENSIONS.
    • On the call we mentioned cl_khr_icd and an AMD offline devices extension. Are there others?
    • What is the criteria we would use to determine whether an extension is a platform extension or a device extension?
    • Could / should we encode this difference in the XML file?
  2. I will update my CTS PR to include the notion of pure platform extensions, which will "fix" the one implementation that is returning cl_khr_icd for the platform but not for the device.
  3. We need to determine whether the current spec text is sufficiently clear, or whether spec changes are needed.

@karolherbst
Copy link
Contributor Author

I think there are certainly a list of extensions one could consider "platform extension" as they don't rely on any hardware capabilities:

  • cl_khr_create_command_queue
  • cl_khr_extended_versioning
  • cl_khr_icd
  • cl_khr_il_program
  • cl_khr_suggested_local_work_size
  • cl_khr_spirv_no_integer_wrap_decoration
  • cl_khr_expect_assume

I think the more interesting aspect here is consistency. What if an implementation also reports "purely platform extensions" as device ones? What if applications start to rely on it as they were developed against such implementation? Should we just state that every platform extension should also be reported as a device one so we enforce consistency across implementations? Or do we want to enforce that "purely platform extensions" won't be reported as device extensions at all? Or should we not care about inconsistency here?

@bashbaug
Copy link
Contributor

I think the more interesting aspect here is consistency. What if an implementation also reports "purely platform extensions" as device ones? What if applications start to rely on it as they were developed against such implementation? Should we just state that every platform extension should also be reported as a device one so we enforce consistency across implementations? Or do we want to enforce that "purely platform extensions" won't be reported as device extensions at all? Or should we not care about inconsistency here?

Hmm, the more I think about this, the more I think the notion of a "platform extension" is unnecessary. Even if it's a little silly, we should just include the "platform extensions" in the device query also (as suggested above), and then we can say that an extension will be included in the query for CL_PLATFORM_EXTENSIONS if and only if it is also included in the query for CL_DEVICE_EXTENSIONS for all of the devices in the platform.

If we don't do this, we're going to need to define exactly what it means to be a "platform extension", and I think we're going to find that challenging and subjective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs WG discussion
Development

No branches or pull requests

2 participants