Skip to content

Rewrite OpenCL device discovery to SYCL. #1401

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Rewrite OpenCL device discovery to SYCL. #1401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e06110
Rewrite OpenCL device discovery to SYCL.
clevels Mar 5, 2020
4882fa6
Update sycl/tools/get_device_count_by_type.cpp
clevels Apr 28, 2020
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
2 changes: 2 additions & 0 deletions sycl/tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ set(CMAKE_CXX_EXTENSIONS OFF)

add_executable(get_device_count_by_type get_device_count_by_type.cpp)
add_dependencies(get_device_count_by_type ocl-headers ocl-icd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_dependencies(get_device_count_by_type ocl-headers ocl-icd)

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot remember why this was added but I ran into situations where the use of the OpenCL::Headers target was not enough. I suggest to test this when removing it, e.g., by starting from a completely fresh repo with and without the OCL headers and Khronos ICD.

target_include_directories(get_device_count_by_type PRIVATE "${sycl_inc_dir}")
target_link_libraries(get_device_count_by_type
PRIVATE sycl
PRIVATE OpenCL::Headers
PRIVATE ${OpenCL_LIBRARIES}
)
Expand Down
50 changes: 13 additions & 37 deletions sycl/tools/get_device_count_by_type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
//
//===----------------------------------------------------------------------===//

#include <CL/cl.h>
#include <CL/cl_ext.h>
#include <CL/sycl.hpp>

#ifdef USE_PI_CUDA
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need CUDA specific code now?

#include <cuda_driver.h>
Expand All @@ -17,6 +16,8 @@
#include <string>
#include <vector>

using namespace cl::sycl;

static const std::string help =
" Help\n"
" Example: ./get_device_count_by_type cpu opencl\n"
Expand Down Expand Up @@ -61,54 +62,29 @@ int main(int argc, char* argv[]) {
}
#endif // USE_PI_CUDA

cl_device_type device_type;
info::device_type device_type;
if (type == "cpu") {
device_type = CL_DEVICE_TYPE_CPU;
device_type = info::device_type::cpu;
} else if (type == "gpu") {
device_type = CL_DEVICE_TYPE_GPU;
device_type = info::device_type::gpu;
} else if (type == "accelerator") {
device_type = CL_DEVICE_TYPE_ACCELERATOR;
device_type = info::device_type::accelerator;
} else if (type == "default") {
device_type = CL_DEVICE_TYPE_DEFAULT;
device_type = info::device_type::automatic;
} else if (type == "all") {
device_type = CL_DEVICE_TYPE_ALL;
device_type = info::device_type::all;
} else {
std::cout << "0:Incorrect device type." << std::endl
<< help << std::endl;
return 0;
}

cl_int iRet = CL_SUCCESS;
cl_uint platformCount = 0;

iRet = clGetPlatformIDs(0, nullptr, &platformCount);
if (iRet != CL_SUCCESS) {
if (iRet == CL_PLATFORM_NOT_FOUND_KHR) {
std::cout << "0:OpenCL runtime not found " << std::endl;
} else {
std::cout << "0:A problem at calling function clGetPlatformIDs count "
<< iRet << std::endl;
}
return 0;
}

std::vector<cl_platform_id> platforms(platformCount);

iRet = clGetPlatformIDs(platformCount, &platforms[0], nullptr);
if (iRet != CL_SUCCESS) {
std::cout << "0:A problem at when calling function clGetPlatformIDs ids " << iRet << std::endl;
return 0;
}

for (cl_uint i = 0; i < platformCount; i++) {
cl_uint deviceCountPart = 0;
iRet = clGetDeviceIDs(platforms[i], device_type, 0, nullptr, &deviceCountPart);
if (iRet == CL_SUCCESS) {
deviceCount += deviceCountPart;
}
std::vector<platform> platforms(platform::get_platforms());
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think it's right approach to use SYCL here.
Instead I suggest removing get_device_count_by_type completely and updating tests so they try to run on all device types so if such a device is not available just print "Skip".

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on not using SYCL RT. For example, if there is a bug in platforms or device implementation, or something triggered by the plugin discovered, all test will fail and could be very difficult to figure out what the problem is.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to run all SYCL LIT testing if SYCL RT platforms/device discovery is broken, i.e. can't detect correctly what's there. I think that for the sake of SYCL LIT testing we should run it on all platforms/devices detected by SYCL RT. There should be a separate single test that verifies that SYCL RT is doing the correct discovery as compared to what's discovered by low-level run-times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on not using SYCL RT. For example, if there is a bug in platforms or device implementation, or something triggered by the plugin discovered, all test will fail and could be very difficult to figure out what the problem is.

To check that the plugin functionality or platform/device implementations are working fine, we can add a test case which checks the listed devices by calling all low level APIs and then checking it w.r.t. the devices returned from plugin. This doesn't need to be checked by all tests.

We would ideally like to start recognizing all devices from all plugins. As that code is being changed, this logic will also need to change in parallel. It is better to use the SYCL APIs, so that it is maintained at one location.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think it's right approach to use SYCL here.
Instead I suggest removing get_device_count_by_type completely and updating tests so they try to run on all device types so if such a device is not available just print "Skip".

I agree, the tests could be written such that they execute on all attached devices and report any error found with any attached device.
Any test needing a special device (opencl only)/ avoiding any specific device testing (like skipping cuda), should use custom device_selectors. We can create common utilities for this purpose. This approach will mean lesser maintenance and customization.

Copy link
Contributor

Choose a reason for hiding this comment

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

To check that the plugin functionality or platform/device implementations are working fine, we can add a test case which checks the listed devices by calling all low level APIs and then checking it w.r.t. the devices returned from plugin. This doesn't need to be checked by all tests.

Seems to me that, somehow, that test should run before any other, so that if that code is broken, no other test is run and we avoid a massive list of failure which can waste a lot of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if LIT has a mode to stop all further testing if a specific test breaks.

for (cl_uint i = 0; i < platforms.size(); i++) {
std::vector<device> result = platforms[i].get_devices(device_type);
deviceCount += result.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the SYCL interface is used, then the CUDA backend check could be done in this loop as well, are you planning to do that?

}

std::cout << deviceCount << ":" << backend << std::endl;

return 0;
}