-
Notifications
You must be signed in to change notification settings - Fork 992
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
xpu/ocl: Minor generic-vendor fixes #2084
base: main
Are you sure you want to change the base?
Conversation
If status != success then it is a non-zero value, but in that case we have zero devices. Return zero instead so we don't index off the end of anything.
We can't do that at this level because it means device indices change based on how we were called.
This isn't the place for it, this is generic code and doesn't rely on Intel's OpenCL platform. There's another check later, conditional on DNNL_VENDOR_INTEL at build time, that has the same effect. On a CometLake system, with recent Mesa using rusticl and iris: [----------] 2 tests from AllEngineKinds/engine_test_t [ RUN ] AllEngineKinds/engine_test_t.TestMultithreading/0 onednn_verbose,v1,info,oneDNN v3.6.0 (commit d36af6e408cd77b84531b5bbe4b6f3874df625a3) onednn_verbose,v1,info,cpu,runtime:OpenMP,nthr:20 onednn_verbose,v1,info,cpu,isa:Intel AVX2 onednn_verbose,v1,info,gpu,runtime:OpenCL WARNING: OpenCL support via iris driver is incomplete. For a complete and conformant OpenCL implementation, use https://github.com/intel/compute-runtime instead onednn_verbose,v1,common,error,runtime,unsupported ocl platform (expected intel got rusticl)
@@ -46,7 +46,7 @@ class engine_factory_t : public impl::engine_factory_t { | |||
std::vector<cl_device_id> ocl_devices; | |||
status_t status | |||
= xpu::ocl::get_devices(&ocl_devices, CL_DEVICE_TYPE_GPU); | |||
if (status != status::success) return status; | |||
if (status != status::success) return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -202,8 +202,6 @@ status_t get_devices(std::vector<cl_device_id> *devices, | |||
OCL_CHECK(clGetPlatformIDs(num_platforms, &platforms[0], nullptr)); | |||
|
|||
for (size_t i = 0; i < platforms.size(); ++i) { | |||
if (!is_intel_platform(platforms[i])) continue; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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. Theis_intel_platform
function merely checks the name of the platform so it doesn't add any dependency on Intel-specific API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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_device
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. TheDNNL_VENDOR_GENERIC
build will get closer to initializing though, and since it already doesn't work that seems like progress.