xpu/ocl: Minor generic-vendor fixes

[nwnk]

First one was found by inspection. Second one is still a bugfix but also a bikeshed: do you want to bake the error cases into the return code or not? I went for treating errors as errors, which means if the OpenCL runtime produces zero devices then that's an empty success. In practice I don't think this is much different but the existing thing was hard to justify.

The last two are arguably behavior changes but they remove vendor invariants from generic code and should not change behavior for the DNNL_VENDOR_INTEL build. The DNNL_VENDOR_GENERIC build will get closer to initializing though, and since it already doesn't work that seems like progress.

[rjoursler]

I agreed that we should not be returning status here, it is not a device count. On the other hand, I think it is important to maintain the distinction between expected errors and unexpected errors (such as the CL_PLATFORM_NOT_FOUND_KHR result from the clGetPlatformIDs call). What if we keep the current get_devices behavior, but use VERROR to log the unexpected error case before returning 0.

[rjoursler]

I agree that getting the intel platform checks out of xpu::ocl makes sense, but we still need some filtering to prevent non-intel platforms from dispatching into the code in src/gpu/intel. Because of this, it seems like we need to add a check for the intel platform before the call to gpu::intel::ocl::engine_create.

In addition, I don't think we can enable the non-intel runtimes to dispatch into this code either as it is not currently supported and there is no testing infrastructure to ensure it works. We will need an RFC on how multiple runtimes should be supported before that can be considered. I get that this functionality is useful for your exploration of oneDNN though, maybe we can add a development variable guard in gpu::intel::ocl::engine_create (similar to here, and documented here) for now?

[densamoilov]

• This change breaks the current contract that get_devices returns only intel devices and therefore it will affect all Intel-specific GPU code that relies on that.
• I don't see a problem with asking get_devices to filter out devices and platforms as it has to be done somewhere anyway. The is_intel_platform function merely checks the name of the platform so it doesn't add any dependency on Intel-specific API.

[nwnk]

@rjoursler:

we still need some filtering to prevent non-intel platforms from dispatching into the code in src/gpu/intel [...] check for the intel platform before the call to gpu::intel::ocl::engine_create.

I like that. Would it make sense to then remove the consistency check in check_device? That's currently the thing preventing us from binding to rusticl on otherwise-Intel hardware, so it needs to be kept somewhere, but I think if we assert is_intel_platform before engine_create we wouldn't need to verify it later.

In addition, I don't think we can enable the non-intel runtimes to dispatch into this code either as it is not currently supported and there is no testing infrastructure to ensure it works.

As of 7cfc834 (a few days old, just happens to be checked out), building the OCL engine for the GENERIC vendor does not produce a working libdnnl.so:

[23/359] Linking CXX executable tests/gtests/test_binary
FAILED: tests/gtests/test_binary 
: && /usr/bin/c++ -fopenmp -fvisibility-inlines-hidden  -Wall -Wno-unknown-pragmas -fvisibility=internal -Wno-ignored-attributes -fPIC -Wformat -Wformat-security -fstack-protector-strong     -Wno-strict-overflow -Wno-maybe-uninitialized -O2 -g -DNDEBUG -pie -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now tests/gtests/CMakeFiles/test_binary.dir/main.cpp.o tests/gtests/CMakeFiles/test_binary.dir/__/test_thread.cpp.o tests/gtests/CMakeFiles/test_binary.dir/test_binary.cpp.o -o tests/gtests/test_binary  -Wl,-rpath,/var/home/ajax/git/oneDNN/build/src  src/libdnnl.so.3.6  tests/gtests/gtest/libdnnl_gtest.a  /usr/lib64/libOpenCL.so  -ldl  -lpthread && :
/usr/bin/ld: tests/gtests/CMakeFiles/test_binary.dir/test_binary.cpp.o: in function `dnnl::ocl_interop::make_memory(dnnl::memory::desc const&, dnnl::engine const&, dnnl::ocl_interop::memory_kind, void*)':
/var/home/ajax/git/oneDNN/include/oneapi/dnnl/dnnl_ocl.hpp:271:(.text._ZN4test11make_memoryERKN4dnnl6memory4descERKNS0_6engineE[_ZN4test11make_memoryERKN4dnnl6memory4descERKNS0_6engineE]+0x6e): undefined reference to `dnnl_ocl_interop_memory_create'
/usr/bin/ld: src/libdnnl.so.3.6: undefined reference to `dnnl::impl::xpu::ocl::get_devices(std::vector<_cl_device_id*, std::allocator<_cl_device_id*> >*, unsigned long, unsigned int)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Even if it would link, the engine factory would only call the rest of the engine ctor if we were built for INTEL, and GENERIC will just throw a runtime error. All of which to say: I agree it ought to be tested once it's enabled, but that configuration is still pretty unreachable.

@densamoilov:

This change breaks the current contract that get_devices returns only intel devices and therefore it will affect all Intel-specific GPU code that relies on that.

Meh? I don't think that's an especially good contract, certainly not in code that doesn't have intel in the pathname, and (per the above) it can't matter yet as the GENERIC configuration does not reach clCreateContext. In the supported INTEL configuration we will still only initialize on cl_devices from the Intel platform, so the only visible change is that the device array would have more things in it. But (afaict) at runtime we only really operate in terms of engines not devices. So it's hard for me to see that there is any reliance on that invariant.

I don't see a problem with asking get_devices to filter out devices and platforms as it has to be done somewhere anyway. The is_intel_platform function merely checks the name of the platform so it doesn't add any dependency on Intel-specific API.

If the list of devices needs to be filtered then the place to do it is in the place that wants the filtered view. If the GENERIC configuration can be made to work, I could easily imagine something like rusticl for local work generation plus pocld to distribute long-lived work units. That means I will want a list of devices from more than one OpenCL platform, from all of them in fact. And unfortunately the "device index" needs to be unique across the engine kind which means if I want to talk to two OpenCL platforms then I need to assign globally unique indexes to their devices anyway. I may as well just collect them all here, and (per the above) apply the platform filter before finishing device init.

[densamoilov]

I don't think that's an especially good contract

My point is, the contract exists and therefore if you want to change it you will have to adjust the code that relies on it.

[rjoursler]

My point is, the contract exists and therefore if you want to change it you will have to adjust the code that relies on it.

For an example, consider this location.

[rjoursler]

I like that. Would it make sense to then remove the consistency check in check_device?

That check looks like it doesn't belongs in xpu::ocl anyway. It should likely be moved to intel::ocl::ocl_gpu_engine_t::init() and use VCONDCHECK to log the creation failure. With this design, there is no need to guard the call to engine_create either, as creation failure will be enforced by the intel::ocl_gpu_engine_t structure.