Clarify value returned for CL_DEVICE_TYPE query

[kpet]

The specification currently states:

The OpenCL device type. Currently supported
values are:
CL_DEVICE_TYPE_CPU, CL_DEVICE_TYPE_GPU, CL_
DEVICE_TYPE_ACCELERATOR, CL_DEVICE_TYPE_DEFAULT,
a combination of the above types, or CL_DEVICE_
TYPE_CUSTOM.

This means that an implementation could return CL_DEVICE_TYPE_CPU|CL_DEVICE_TYPE_GPU, CL_DEVICE_TYPE_ACCELERATOR|CL_DEVICE_TYPE_DEFAULT. What is the rationale for allowing CPU|GPU for example?

Note that the spec currently doesn't describe anything CPU- or GPU-specific.

[Oblomov]

The possibility to combine flag values was mostly used by platforms to specify which device was the default for that platform (e.g. returning CL_DEVICE_TYPE_CPU|CL_DEVICE_TYPE_DEFAULT), but even that is being phased out, since a lot of client code erroneously checks for device types by equality (e.g. it tries to skip the CPU device during enumeration by checking if the returned value is CL_DEVICE_TYPE_CPU, rather than checking only the corresponding bit).

As for a mixed-type device, it wouldn't be inconceivable to have a single piece of hardware acting as both the CPU and GPU of the host, even though there aren't devices that do that AFAIK.

[bashbaug]

that is being phased out, since a lot of client code erroneously checks for device types by equality

Interesting. We are considering adding new device types to distinguish between different types of ACCELERATOR devices, or integrated / discrete GPU devices, for example. If we did this, we would recommend that these devices continue to return the "old" device type for backwards compatibility in addition to the newer more detailed device type, e.g. CL_DEVICE_TYPE_FANCY_NEW_ACCELERATOR | CL_DEVICE_TYPE_ACCELERATOR. This obviously won't work, though, if client code is checking the device types by equality. Should we consider adding the additional device types via a separate query instead?

[Oblomov]

Personally, I still prefer the “device type as bitmap” approach precisely because of the kind of possibilities it offered, including something like what @bashbaug suggested. I would focus on improving the documentation about it (e.g. adding examples showing that to check if the device is a GPU one should use ret & CL_DEVICE_TYPE_GPU), fixing clients (at least FLOSS ones), and maybe add some companion macros (CL_DEVICE_IS_CPU()) to wrap the check.

[bashbaug]

maybe add some companion macros (CL_DEVICE_IS_CPU()) to wrap the check.

This is a great idea!

[kpet]

I agree that having the ability to report multiple types can be interesting and I agree that the macros are a good idea but at the same time I can see two challenges with this:

• This feature was widely misunderstood (many applications test for equality) and I doubt fixing the world would be easy.
• If a device can be all types simultaneously, what can an application do with this information? There is currently no specified behaviour other than for CL_DEVICE_CUSTOM (which is incidently the only value that can't be reported along with others, even with CL_DEVICE_TYPE_DEFAULT; can't a custom device be the default device?). It seems to me that an application can only make use of this by making assumptions on the behaviour of the implementation.

[Oblomov]

Some additional remarks (not necessarily in defense of either solution):

• if the value is not a bitfield, then, CL_DEVICE_TYPE_DEFAULT has no reason to exist at all;

• while it's true that feauture has been widely misunderstood, one could argue that this is mostly a documentation issue; better highlighting of the nature of the value, including examples etc can help prevent future mistakes; as for the old ones:

  1. all free software can be fixed to check things correctly;
  2. maintained closed-source software can (arguably) also be fixed;
  3. unmaintained closed-source software can be live-patched, or the ICD could have a “broken software compatibility mode” where the device info calls for CL_DEVICE_TYPE filtering out the CPU and GPU bits only: such a software is extremely unlikely to know what to do with anything else anyway;

• in fact, more in general, one could argue: why does this field even exist in the first place? Almost all of the information that one could “second guess” from knowing the device is a CPU or a GPU or an accelerator would be better served by other, more specific fields (aside from the ones already present, including the additional ones already exposed by some vendor extensions): does this device prefer large or small workgroups? is it best tuned for task-based or data-based parallelism? does it benefit from overcommitting? what are the peak theoretical bandwidth and latency of its memory? if memory is not unified with host, what are the bandwidth and latency of host/device data transfers?

[bashbaug]

I think this is a great observation:

if the value is not a bitfield, then, CL_DEVICE_TYPE_DEFAULT has no reason to exist at all;

I also think this is a great idea:

the ICD loader could have a “broken software compatibility mode” where the device info calls for CL_DEVICE_TYPE filtering out the CPU and GPU bits only

I've been meaning to write up some other ideas to enhance the ICD loader anyhow. Do we know apps for sure that will fail or otherwise behave badly with unconventional return values for CL_DEVICE_TYPE? How highly should something like this be prioritized?

[Oblomov]

I've had some time to think about this, and the CL_DEVICE_TYPE_DEFAULT cannot really be removed, since it may be used e.g. in the clGetContextFromType() call, for example, to specify the default device type for the platform without additional requirements, or in clGetDeviceIDs() for the same purpose. So either the whole concept of device type gets removed, which would require obsoleting a part of the API that is very much used, or the flags as value need to be kept (modulo the “legacy support fix I mentioned in the previous comment”).

Finding applications that fail to load due to “extended” return from the device type query has actually become rather hard, because most platforms have been “fixed” (in the cat sense, I'm afraid), to only return one device type, generally by not adding the CL_DEVICE_TYPE_DEFAULT. See for example this pocl commit.

I'm actually not sure how widespread the wrong detection is. I mostly use free software, and when I find the wrong usage I submit a patch to fix it. I've done a quick search on GitHub right now, and I've seen both use cases. Interestingly, using the wrong or right form doesn't seem to depend on the “professionality” of the project. For example, ffmpeg uses the correct one, whereas Intel's TBB examples use the wrong ones (example 1, example 2) (although one could argue that in Intel's case, at least for the second example, they're specifically testing against their own platform so there's no need to use the correct way to check the device type —still, poor form, at least in the first example).

[bashbaug]

We've had some related confusion around CL_DEVICE_TYPE in a SYCL code review:

https://github.com/intel/llvm/pull/1694/files#r427952598

To fix this issue and to clear up the confusion, here is my proposal:

• Say explicitly that the device type is purely informational, with no semantic meaning.

• Additionally, say explicitly that a device can identify as multiple device types if applicable. Note, I think the description for the CL_DEVICE_TYPE query already allows this, but it could be strengthened to avoid confusion.

• Weaken the text describing each device type. I think it's fine to provide examples using text like "such as", but I don't think we should require that a CPU device is the host processor, or that a GPU device implements a 3D API, or that an accelerator device uses a peripheral interconnect, for example.

• Perhaps we should require that one and only one device in a platform must identify as CL_DEVICE_DEFAULT? I think this makes sense and we could write a CTS test for it. The main complexity if we require this behavior would be environment variables that hide devices - see, for example, this ICD loader PR.

If this sounds good I will start drafting spec changes for this proposal.

[bashbaug]

The PR with the proposed spec text above is #299.