-
Notifications
You must be signed in to change notification settings - Fork 989
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,21 +190,18 @@ static bool is_intel_platform(cl_platform_id platform) { | |
} | ||
|
||
status_t get_devices(std::vector<cl_device_id> *devices, | ||
cl_device_type device_type, cl_uint vendor_id /* = 0x8086 */) { | ||
cl_device_type device_type) { | ||
cl_uint num_platforms = 0; | ||
|
||
cl_int err = clGetPlatformIDs(0, nullptr, &num_platforms); | ||
// No platforms - a valid scenario | ||
if (err == CL_PLATFORM_NOT_FOUND_KHR) return status::success; | ||
if (err == CL_PLATFORM_NOT_FOUND_KHR) return status::runtime_error; | ||
|
||
OCL_CHECK(err); | ||
|
||
std::vector<cl_platform_id> platforms(num_platforms); | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I agree that getting the intel platform checks out of 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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
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.
Meh? I don't think that's an especially good contract, certainly not in code that doesn't have
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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For an example, consider this location. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That check looks like it doesn't belongs in |
||
cl_uint num_devices = 0; | ||
cl_int err = clGetDeviceIDs( | ||
platforms[i], device_type, 0, nullptr, &num_devices); | ||
|
@@ -219,17 +216,16 @@ status_t get_devices(std::vector<cl_device_id> *devices, | |
OCL_CHECK(clGetDeviceIDs(platforms[i], device_type, num_devices, | ||
&plat_devices[0], nullptr)); | ||
|
||
// Use the devices for the requested vendor only. | ||
for (size_t j = 0; j < plat_devices.size(); ++j) { | ||
cl_uint v_id; | ||
OCL_CHECK(clGetDeviceInfo(plat_devices[j], CL_DEVICE_VENDOR_ID, | ||
sizeof(cl_uint), &v_id, nullptr)); | ||
if (v_id == vendor_id) { devices->push_back(plat_devices[j]); } | ||
devices->push_back(plat_devices[j]); | ||
} | ||
} | ||
} | ||
// No devices found but still return success | ||
return status::success; | ||
|
||
if (devices->size() != 0) | ||
return status::success; | ||
|
||
return status::runtime_error; | ||
} | ||
|
||
status_t get_devices(std::vector<cl_device_id> *devices, | ||
|
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 theCL_PLATFORM_NOT_FOUND_KHR
result from theclGetPlatformIDs
call). What if we keep the currentget_devices
behavior, but use VERROR to log the unexpected error case before returning 0.