Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sycl/plugins/opencl/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@
#TODO: Currently, the pi.h header is common between sycl and plugin library sources.
#This can be changed by copying the pi.h file in the plugins project.

find_package(Threads REQUIRED)

add_sycl_plugin(opencl
SOURCES
"${sycl_inc_dir}/sycl/detail/pi.h"
"pi_opencl.cpp"
LIBRARIES
OpenCL-ICD
Threads::Threads
)

set_target_properties(pi_opencl PROPERTIES LINKER_LANGUAGE CXX)
234 changes: 227 additions & 7 deletions sycl/plugins/opencl/pi_opencl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <sycl/detail/cl.h>
#include <sycl/detail/iostream_proxy.hpp>
#include <sycl/detail/pi.h>
#include <sycl/detail/spinlock.hpp>

#include <algorithm>
#include <cassert>
Expand All @@ -29,8 +30,21 @@
#include <memory>
#include <sstream>
#include <string>
#include <thread>
#include <vector>

#include "pi_utils.hpp"

static const bool ExposeCSliceInAffinityPartitioning = [] {
const char *Flag =
std::getenv("SYCL_PI_OPENCL_EXPOSE_CSLICE_IN_AFFINITY_PARTITIONING");
return Flag ? std::atoi(Flag) != 0 : false;
}();

#define PI_ASSERT(condition, error) \
if (!(condition)) \
return error;

#define CHECK_ERR_SET_NULL_RET(err, ptr, reterr) \
if (err != CL_SUCCESS) { \
if (ptr != nullptr) \
Expand Down Expand Up @@ -258,9 +272,49 @@ static pi_result USMSetIndirectAccess(pi_kernel kernel) {

extern "C" {

// Helper function
static _pi_device::device_level nextLevel(const _pi_device::device_level currLevel) {
switch (currLevel) {
case _pi_device::ROOTDEVICE :
return _pi_device::SUBDEVICE;
case _pi_device::SUBDEVICE:
return _pi_device::SUBSUBDEVICE;
default:
return _pi_device::INVALID;
}
}

// Return device level
// _pi_device::ROOTDEVICE -> root device
// _pi_device::SUBDEVICE -> sub-device
// _pi_device::SUBSUBDEVICE -> sub-sub-device (CCS)
// _pi_device::INVALID -> invalid device
_pi_device::device_level getLevel(pi_device device) {
if (!device)
return _pi_device::INVALID;
cl_device_id parentId = nullptr;
clGetDeviceInfo(cast<cl_device_id>(device), CL_DEVICE_PARENT_DEVICE,
sizeof(cl_device_id), &parentId, NULL);
if (parentId == nullptr)
return _pi_device::ROOTDEVICE;
cl_device_id parentParentId = nullptr;
clGetDeviceInfo(parentId, CL_DEVICE_PARENT_DEVICE, sizeof(cl_device_id),
&parentParentId, NULL);
if (parentParentId == nullptr)
return _pi_device::SUBDEVICE;
cl_device_id parentParentParentId = nullptr;
clGetDeviceInfo(parentParentId, CL_DEVICE_PARENT_DEVICE, sizeof(cl_device_id),
&parentParentParentId, NULL);
if (parentParentParentId == nullptr)
return _pi_device::SUBSUBDEVICE;
return _pi_device::INVALID;
}

pi_result piDeviceGetInfo(pi_device device, pi_device_info paramName,
size_t paramValueSize, void *paramValue,
size_t *paramValueSizeRet) {
PI_ASSERT(device, PI_ERROR_INVALID_DEVICE);
ReturnHelper ReturnValue(paramValueSize, paramValue, paramValueSizeRet);
switch (paramName) {
// TODO: Check regularly to see if support in enabled in OpenCL.
// Intel GPU EU device-specific information extensions.
Expand Down Expand Up @@ -342,6 +396,97 @@ pi_result piDeviceGetInfo(pi_device device, pi_device_info paramName,
return PI_SUCCESS;
}

case PI_DEVICE_INFO_PARTITION_PROPERTIES: {
Comment thread
aelovikov-intel marked this conversation as resolved.
// SYCL spec says: if this SYCL device cannot be partitioned into at least
// two sub devices then the returned vector must be empty.
pi_uint32 numSubDevices = 0;
if (device->level == _pi_device::INVALID) // level not yet updated
device->level = getLevel(device);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this lazy? Can we update when device is created?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually do update this when device is created. So, getLevel will mostly not be called here. I am just using this as a fail-safe. But, thinking of it, I am not able to formulate a scenario where device->level can be invalid once it has been created. I can take out this code if needed. Thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have an assert here then. The laziness complicates the code unnecessarily.


auto ReturnHelper = [&](auto... Partitions) {
struct {
pi_device_partition_property Arr[sizeof...(Partitions) + 1];
} PartitionProperties = {{Partitions..., 0}};
return ReturnValue(PartitionProperties);
};

switch (device->level) {
case _pi_device::ROOTDEVICE: {
clGetDeviceInfo(
cast<cl_device_id>(device), CL_DEVICE_PARTITION_MAX_SUB_DEVICES,
sizeof(numSubDevices), &numSubDevices, nullptr);
if (numSubDevices < 2) {
return ReturnValue(pi_device_partition_property{0});
}
return ReturnHelper(PI_DEVICE_PARTITION_BY_AFFINITY_DOMAIN);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend restructuring this code slightly to eliminate a few assumptions and improve robustness:

  1. Instead of assuming that when a root device can be partitioned that it can be partitioned BY_AFFINITY_DOMAIN, consider querying CL_DEVICE_PARTITION_PROPERTIES for the device instead. I don't expect that PVC will support additional partition properties, but if it ever does then this code will not need to be updated.

  2. This one is more important: We should look for a command queue family with more than one queue whenever the passed-in device does not natively support partitioning into sub-devices, regardless whether the passed-in device is a root device or a sub-device. When/if we switch to "tiles as devices", the passed-in device may support partitioning by CSlice even though it is a root-device.

@asudarsa asudarsa Jan 31, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR tries to mimic L0 plugin behavior for piDeviceGetInfo and piDevicepartition. One input I have: We submit the OpenCL plugin changes as is. Then, we modify OpenCL plugin implementation to make it more robust in a separate PR.

}
case _pi_device::SUBDEVICE: {
// find out number of CCSes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// find out number of CCSes
// Find out number of CCSes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be done in next patch. Thanks

bool supported = false;
cl_int ret_err = CL_SUCCESS;
ret_err =
checkDeviceExtensions(cast<cl_device_id>(device),
{"cl_intel_command_queue_families"}, supported);
if (ret_err != CL_SUCCESS)
return static_cast<pi_result>(ret_err);
if (!supported) {
std::cout
<< "This device does not support cl_intel_command_queue_families"
<< std::endl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like those debug prints need to be removed/guarded by debug macros.

return ReturnValue(pi_device_partition_property{0});
}
cl_queue_family_properties_intel qfprops[3];
size_t qsize = 0;
clGetDeviceInfo(
cast<cl_device_id>(device), CL_DEVICE_QUEUE_FAMILY_PROPERTIES_INTEL,
sizeof(qfprops), qfprops, &qsize);
qsize = qsize / sizeof(cl_queue_family_properties_intel);
for (size_t q = 0; q < qsize; q++) {
if (qfprops[q].capabilities == CL_QUEUE_DEFAULT_CAPABILITIES_INTEL &&
qfprops[q].count > numSubDevices) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this condition mean? Can we have numSubDevices not equal to zero?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covering a scenario where multiple families have 'DEFAULT' capabilities and we are trying to get the handle to the family with maximum number of dub-devices.

numSubDevices = qfprops[q].count;
}
}
if (numSubDevices < 2) {
return ReturnValue(pi_device_partition_property{0});
}
if (ExposeCSliceInAffinityPartitioning) {
return ReturnHelper(PI_EXT_INTEL_DEVICE_PARTITION_BY_CSLICE,
PI_DEVICE_PARTITION_BY_AFFINITY_DOMAIN);
}
return ReturnHelper(PI_EXT_INTEL_DEVICE_PARTITION_BY_CSLICE);
}
default:
return ReturnValue(pi_device_partition_property{0});
}
}
case PI_DEVICE_INFO_PARTITION_AFFINITY_DOMAIN:
return ReturnValue(pi_device_affinity_domain{
PI_DEVICE_AFFINITY_DOMAIN_NUMA |
PI_DEVICE_AFFINITY_DOMAIN_NEXT_PARTITIONABLE});
Comment on lines +499 to +501

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to forward this query to the OpenCL driver via CL_DEVICE_PARTITION_AFFINITY_DOMAIN rather than hard-coding NUMA | NEXT_PARTITIONABLE, because some devices may not support partitioning by affinity domains.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine postponing this if we need to, but "fixing" it should be as simple as removing these lines and letting the default case handle the PI_DEVICE_INFO_PARTITION_AFFINITY_DOMAIN query.

case PI_DEVICE_INFO_PARTITION_TYPE: {
if (device->level == _pi_device::INVALID) // level not updated yet
device->level = getLevel(device);
// For root-device there is no partitioning to report.
if (device->isRootDevice())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we should either drop lines 468/469 (getLevel) or convert ifs into a switch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah..good catch. I will drop lines 468/469.

return ReturnValue(pi_device_partition_property{0});
if (device->isSubDevice()) {
struct {
pi_device_partition_property Arr[3];
} PartitionProperties = {{PI_DEVICE_PARTITION_BY_AFFINITY_DOMAIN,
PI_DEVICE_AFFINITY_DOMAIN_NEXT_PARTITIONABLE,
0}};
return ReturnValue(PartitionProperties);
}
if (device->isSubSubDevice()) {
struct {
pi_device_partition_property Arr[2];
} PartitionProperties = {{PI_EXT_INTEL_DEVICE_PARTITION_BY_CSLICE, 0}};
return ReturnValue(PartitionProperties);
}
return ReturnValue(pi_device_partition_property{0});
}

default:
cl_int result = clGetDeviceInfo(
cast<cl_device_id>(device), cast<cl_device_info>(paramName),
Expand All @@ -350,6 +495,63 @@ pi_result piDeviceGetInfo(pi_device device, pi_device_info paramName,
}
}

pi_result piDevicePartition(pi_device device,
const pi_device_partition_property *properties,
pi_uint32 num_devices, pi_device *out_devices,
pi_uint32 *out_num_devices) {
cl_int result = CL_DEVICE_NOT_FOUND;
if (device->level == _pi_device::INVALID) // level not updated yet
device->level = getLevel(device);
// For root-device there is no partitioning to report.
if (device->isRootDevice()) {
result = clCreateSubDevices(
cast<cl_device_id>(device),
cast<const cl_device_partition_property *>(properties),
cast<cl_uint>(num_devices), cast<cl_device_id *>(out_devices),
cast<cl_uint *>(out_num_devices));
if (out_devices) {
for (uint32_t i = 0; i < *out_num_devices; ++i) {
out_devices[i]->level = nextLevel(device->level);
}
}
} else if (device->isSubDevice()) {
cl_queue_family_properties_intel qfprops[3];
size_t qsize = 0;
pi_uint32 family = 0;
cl_uint partitionMaxSubDevices = 0;
clGetDeviceInfo(
cast<cl_device_id>(device), CL_DEVICE_QUEUE_FAMILY_PROPERTIES_INTEL,
sizeof(qfprops), qfprops, &qsize);
qsize = qsize / sizeof(cl_queue_family_properties_intel);
for (size_t q = 0; q < qsize; q++) {
if (qfprops[q].capabilities == CL_QUEUE_DEFAULT_CAPABILITIES_INTEL &&
qfprops[q].count > partitionMaxSubDevices) {
family = q;
partitionMaxSubDevices = qfprops[q].count;
}
}
*out_num_devices = partitionMaxSubDevices;
if (out_devices) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd do an early return here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'else' case here will pipe down to the final 'if' condition in this function. So an early return might need some code duplication.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have

if (out_devices) {
  for () {
  }
}
return PI_SUCCESS;

I suggested

if (!out_devices)
  return PI_SUCCESS; // We're done
  
// Fill out "out_devices" out param
for () {
}
return PI_SUCCESS;

What "else" case are you referring to?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry...I meant the 'branch not taken' case. Before we eventually exit from the function we have to check if devices were found. Then we need to set *out_num_devices to 0 if there are no devices found. We cannot just return PI_SUCCESS.

@asudarsa asudarsa Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.. I actually removed the PI_SUCCESS by mistake. I am adding it back. I will add this early return in upcoming commit. Thanks

for (uint32_t i = 0; i < *out_num_devices; ++i) {
pi_device cloneDevice(device);
out_devices[i] = cloneDevice;
out_devices[i]->level = nextLevel(device->level);
out_devices[i]->family = family;
out_devices[i]->index = i % (*out_num_devices);
}
}
return PI_SUCCESS;
}
// Absorb the CL_DEVICE_NOT_FOUND and just return 0 in out_num_devices

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Absorb the CL_DEVICE_NOT_FOUND and just return 0 in out_num_devices
// Absorb the CL_DEVICE_NOT_FOUND and just return 0 in out_num_devices.

if (result == CL_DEVICE_NOT_FOUND) {
std::cout << "Device not found\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More debug prints that need a cleanup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. removed it. Thanks

assert(out_num_devices != 0);
*out_num_devices = 0;
return PI_SUCCESS;
}
return cast<pi_result>(result);
}

pi_result piPlatformsGet(pi_uint32 num_entries, pi_platform *platforms,
pi_uint32 *num_platforms) {
cl_int result = clGetPlatformIDs(cast<cl_uint>(num_entries),
Expand Down Expand Up @@ -381,6 +583,9 @@ pi_result piDevicesGet(pi_platform platform, pi_device_type device_type,
cast<cl_uint>(num_entries), cast<cl_device_id *>(devices),
cast<cl_uint *>(num_devices));

for (pi_uint32 i = 0; i < num_entries; ++i) {
devices[i]->level = _pi_device::ROOTDEVICE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this in place I'm really surprised we need lazy initialization for this ->level in the code earlier in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. However, earlier in the code, we will actually not call the getLevel function as we are already setting the level here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have actually removed getLevel function. We can populate the level info at the time of device creation.

}
// Absorb the CL_DEVICE_NOT_FOUND and just return 0 in num_devices
if (result == CL_DEVICE_NOT_FOUND) {
assert(num_devices != 0);
Expand Down Expand Up @@ -482,6 +687,7 @@ pi_result piextQueueCreate(pi_context Context, pi_device Device,
return PI_ERROR_INVALID_VALUE;
return piQueueCreate(Context, Device, Flags, Queue);
}

pi_result piQueueCreate(pi_context context, pi_device device,
pi_queue_properties properties, pi_queue *queue) {
assert(queue && "piQueueCreate failed, queue argument is null");
Expand Down Expand Up @@ -518,12 +724,26 @@ pi_result piQueueCreate(pi_context context, pi_device device,
return cast<pi_result>(ret_err);
}

cl_queue_properties CreationFlagProperties[] = {
CL_QUEUE_PROPERTIES,
cast<cl_command_queue_properties>(properties) & SupportByOpenCL, 0};
*queue = cast<pi_queue>(clCreateCommandQueueWithProperties(
cast<cl_context>(context), cast<cl_device_id>(device),
CreationFlagProperties, &ret_err));
if (device->level == _pi_device::SUBSUBDEVICE) {
cl_queue_properties CreationFlagProperties[] = {
CL_QUEUE_PROPERTIES,
cast<cl_command_queue_properties>(properties) & SupportByOpenCL,
CL_QUEUE_FAMILY_INTEL,
device->family,
CL_QUEUE_INDEX_INTEL,
device->index,
0};
*queue = cast<pi_queue>(clCreateCommandQueueWithProperties(
cast<cl_context>(context), cast<cl_device_id>(device),
CreationFlagProperties, &ret_err));
} else {
cl_queue_properties CreationFlagProperties[] = {
CL_QUEUE_PROPERTIES,
cast<cl_command_queue_properties>(properties) & SupportByOpenCL, 0};
*queue = cast<pi_queue>(clCreateCommandQueueWithProperties(
cast<cl_context>(context), cast<cl_device_id>(device),
CreationFlagProperties, &ret_err));
}
return cast<pi_result>(ret_err);
}

Expand Down Expand Up @@ -1689,7 +1909,7 @@ pi_result piPluginInit(pi_plugin *PluginInit) {
// Device
_PI_CL(piDevicesGet, piDevicesGet)
_PI_CL(piDeviceGetInfo, piDeviceGetInfo)
_PI_CL(piDevicePartition, clCreateSubDevices)
_PI_CL(piDevicePartition, piDevicePartition)
_PI_CL(piDeviceRetain, clRetainDevice)
_PI_CL(piDeviceRelease, clReleaseDevice)
_PI_CL(piextDeviceSelectBinary, piextDeviceSelectBinary)
Expand Down
Loading