From 7cee465dc566ae1a726472344ea1bb1165c02c88 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Wed, 19 May 2021 07:23:31 -0500 Subject: [PATCH 1/4] Fixes #447 DPCLTDeviceMgr_GetDevices should not rely of the cached unordered map of root sycl devices to sycl contexts. Because unordered map can change the ordering. Changed implementation to iterate over device::get_devices instead. Confirmation of the fix: ``` In [1]: import dpctl In [2]: d0 = dpctl.SyclDevice("gpu:0") In [3]: d0.filter_string Out[3]: 'opencl:gpu:0' In [4]: [i for i, di in enumerate(dpctl.get_devices(dpctl.backend_type.all, dpctl.device_type.gpu)) if di == d0] Out[4]: [0] In [5]: d0 == dpctl.SyclDevice(d0.filter_string) Out[5]: True In [6]: d1 = dpctl.SyclDevice("gpu:1") In [7]: d1.filter_string Out[7]: 'level_zero:gpu:0' In [8]: [i for i, di in enumerate(dpctl.get_devices(dpctl.backend_type.all, dpctl.device_type.gpu)) if di == d1] Out[8]: [1] In [9]: d1 == dpctl.SyclDevice(d1.filter_string) Out[9]: True ``` --- dpctl-capi/source/dpctl_sycl_device_manager.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/dpctl-capi/source/dpctl_sycl_device_manager.cpp b/dpctl-capi/source/dpctl_sycl_device_manager.cpp index 364585896b..1904d0d3ff 100644 --- a/dpctl-capi/source/dpctl_sycl_device_manager.cpp +++ b/dpctl-capi/source/dpctl_sycl_device_manager.cpp @@ -146,15 +146,18 @@ DPCTLDeviceMgr_GetDevices(int device_identifier) } catch (std::bad_alloc const &ba) { return nullptr; } - auto &cache = DeviceCacheBuilder::getDeviceCache(); + const auto &root_devices = device::get_devices(); + default_selector mRanker; - for (const auto &entry : cache) { + for (const auto &root_device : root_devices) { + if (mRanker(root_device) < 0) + continue; auto Bty(DPCTL_SyclBackendToDPCTLBackendType( - entry.first.get_platform().get_backend())); + root_device.get_platform().get_backend())); auto Dty(DPCTL_SyclDeviceTypeToDPCTLDeviceType( - entry.first.get_info())); + root_device.get_info())); if ((device_identifier & Bty) && (device_identifier & Dty)) { - Devices->emplace_back(wrap(new device(entry.first))); + Devices->emplace_back(wrap(new device(root_device))); } } // the wrap function is defined inside dpctl_vector_templ.cpp From 00cb985e53f3389c98eb21c61cf090b223f51e4c Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Wed, 19 May 2021 07:26:24 -0500 Subject: [PATCH 2/4] Added tests for #447 --- dpctl/tests/test_sycl_device_factory.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/dpctl/tests/test_sycl_device_factory.py b/dpctl/tests/test_sycl_device_factory.py index 8b269767ff..dac2c8b7e0 100644 --- a/dpctl/tests/test_sycl_device_factory.py +++ b/dpctl/tests/test_sycl_device_factory.py @@ -177,10 +177,17 @@ def test_get_devices_with_device_type_enum(device_type): def test_get_devices_with_device_type_str(device_type_str): - devices = dpctl.get_devices(device_type=device_type_str) - if len(devices): - d = string_to_device_type(device_type_str) - check_if_device_type_matches(devices, d) + num_devices = dpctl.get_num_devices(device_type=device_type_str) + if num_devices > 0: + devices = dpctl.get_devices(device_type=device_type_str) + assert len(devices) == num_devices + dty = string_to_device_type(device_type_str) + check_if_device_type_matches(devices, dty) check_if_device_type_is_valid(devices) + # check for consistency of ordering between filter selector + # where backend is omitted, but device type and id is specified + for i in range(num_devices): + dev = dpctl.SyclDevice(":".join((device_type_str, str(i)))) + assert dev == devices[i] else: pytest.skip() From bb9022d09e0561f5c97416e5f091da4a21890ff5 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Wed, 19 May 2021 10:15:15 -0500 Subject: [PATCH 3/4] Added tests for consistency between ordering of GetDevices and filter selector --- dpctl-capi/tests/test_sycl_device_manager.cpp | 78 +++++++++++++++++-- 1 file changed, 72 insertions(+), 6 deletions(-) diff --git a/dpctl-capi/tests/test_sycl_device_manager.cpp b/dpctl-capi/tests/test_sycl_device_manager.cpp index 4dc668c98a..52811f7fea 100644 --- a/dpctl-capi/tests/test_sycl_device_manager.cpp +++ b/dpctl-capi/tests/test_sycl_device_manager.cpp @@ -24,10 +24,12 @@ /// //===----------------------------------------------------------------------===// +#include "../helper/include/dpctl_utils_helper.h" #include "dpctl_sycl_device_interface.h" #include "dpctl_sycl_device_manager.h" #include "dpctl_sycl_device_selector_interface.h" #include +#include struct TestDPCTLDeviceManager : public ::testing::TestWithParam { @@ -81,12 +83,12 @@ INSTANTIATE_TEST_SUITE_P(DeviceMgrFunctions, "opencl:cpu:0", "level_zero:gpu:0")); -struct TestDPCTLDeviceVector : public ::testing::TestWithParam +struct TestDPCTLGetDevices : public ::testing::TestWithParam { DPCTLDeviceVectorRef DV = nullptr; size_t nDevices = 0; - TestDPCTLDeviceVector() + TestDPCTLGetDevices() { EXPECT_NO_FATAL_FAILURE(DV = DPCTLDeviceMgr_GetDevices(GetParam())); EXPECT_TRUE(DV != nullptr); @@ -100,14 +102,14 @@ struct TestDPCTLDeviceVector : public ::testing::TestWithParam } } - ~TestDPCTLDeviceVector() + ~TestDPCTLGetDevices() { EXPECT_NO_FATAL_FAILURE(DPCTLDeviceVector_Clear(DV)); EXPECT_NO_FATAL_FAILURE(DPCTLDeviceVector_Delete(DV)); } }; -TEST_P(TestDPCTLDeviceVector, ChkGetAt) +TEST_P(TestDPCTLGetDevices, ChkGetAt) { for (auto i = 0ul; i < nDevices; ++i) { DPCTLSyclDeviceRef DRef = nullptr; @@ -118,14 +120,18 @@ TEST_P(TestDPCTLDeviceVector, ChkGetAt) INSTANTIATE_TEST_SUITE_P( GetDevices, - TestDPCTLDeviceVector, + TestDPCTLGetDevices, ::testing::Values(DPCTLSyclBackendType::DPCTL_HOST, DPCTLSyclBackendType::DPCTL_LEVEL_ZERO, DPCTLSyclBackendType::DPCTL_OPENCL, DPCTLSyclBackendType::DPCTL_OPENCL | DPCTLSyclDeviceType::DPCTL_GPU)); -TEST(TestDPCTLDeviceVector, ChkDPCTLDeviceVectorCreate) +struct TestDPCTLDeviceVector : public ::testing::Test +{ +}; + +TEST_F(TestDPCTLDeviceVector, ChkDPCTLDeviceVectorCreate) { DPCTLDeviceVectorRef DVRef = nullptr; size_t nDevices = 0; @@ -135,3 +141,63 @@ TEST(TestDPCTLDeviceVector, ChkDPCTLDeviceVectorCreate) EXPECT_TRUE(nDevices == 0); EXPECT_NO_FATAL_FAILURE(DPCTLDeviceVector_Delete(DVRef)); } + +struct TestDPCTLGetDevicesOrdering : public ::testing::TestWithParam +{ + DPCTLDeviceVectorRef DV = nullptr; + size_t nDevices = 0; + + TestDPCTLGetDevicesOrdering() + { + const int device_type_mask = + (GetParam() & DPCTLSyclDeviceType::DPCTL_ALL) | + DPCTLSyclBackendType::DPCTL_ALL_BACKENDS; + EXPECT_NO_FATAL_FAILURE( + DV = DPCTLDeviceMgr_GetDevices(device_type_mask)); + EXPECT_TRUE(DV != nullptr); + EXPECT_NO_FATAL_FAILURE(nDevices = DPCTLDeviceVector_Size(DV)); + } + + void SetUp() + { + if (!nDevices) { + GTEST_SKIP_("Skipping as no devices returned for identifier"); + } + } + + ~TestDPCTLGetDevicesOrdering() + { + EXPECT_NO_FATAL_FAILURE(DPCTLDeviceVector_Clear(DV)); + EXPECT_NO_FATAL_FAILURE(DPCTLDeviceVector_Delete(DV)); + } +}; + +TEST_P(TestDPCTLGetDevicesOrdering, ChkConsistencyWithFilterSelector) +{ + for (auto i = 0ul; i < nDevices; ++i) { + DPCTLSyclDeviceType Dty; + std::string fs_device_type, fs; + DPCTLSyclDeviceRef DRef = nullptr, D0Ref = nullptr; + DPCTLSyclDeviceSelectorRef DSRef = nullptr; + EXPECT_NO_FATAL_FAILURE(DRef = DPCTLDeviceVector_GetAt(DV, i)); + EXPECT_NO_FATAL_FAILURE(Dty = DPCTLDevice_GetDeviceType(DRef)); + EXPECT_NO_FATAL_FAILURE( + fs_device_type = DPCTL_DeviceTypeToStr( + DPCTL_DPCTLDeviceTypeToSyclDeviceType(Dty))); + EXPECT_NO_FATAL_FAILURE(fs = fs_device_type + ":" + std::to_string(i)); + EXPECT_NO_FATAL_FAILURE(DSRef = DPCTLFilterSelector_Create(fs.c_str())); + EXPECT_NO_FATAL_FAILURE(D0Ref = DPCTLDevice_CreateFromSelector(DSRef)); + EXPECT_NO_FATAL_FAILURE(DPCTLDeviceSelector_Delete(DSRef)); + EXPECT_TRUE(DPCTLDevice_AreEq(DRef, D0Ref)); + EXPECT_NO_FATAL_FAILURE(DPCTLDevice_Delete(D0Ref)); + EXPECT_NO_FATAL_FAILURE(DPCTLDevice_Delete(DRef)); + } +} + +INSTANTIATE_TEST_SUITE_P( + GetDevices, + TestDPCTLGetDevicesOrdering, + ::testing::Values(DPCTLSyclDeviceType::DPCTL_HOST_DEVICE, + DPCTLSyclDeviceType::DPCTL_ACCELERATOR, + DPCTLSyclDeviceType::DPCTL_GPU, + DPCTLSyclDeviceType::DPCTL_CPU)); From e19e0f7326a65ed042822f914b304d13feb9bb07 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Wed, 19 May 2021 13:15:34 -0500 Subject: [PATCH 4/4] Changes to int values of DeviceType and BackendType enums DeviceType enums now are [0, 8), BackendType enums have lower 16 bits zero, and span [1<<16, 1<<20) --- dpctl-capi/include/dpctl_sycl_enum_types.h | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/dpctl-capi/include/dpctl_sycl_enum_types.h b/dpctl-capi/include/dpctl_sycl_enum_types.h index 5148f23a56..89f9b11743 100644 --- a/dpctl-capi/include/dpctl_sycl_enum_types.h +++ b/dpctl-capi/include/dpctl_sycl_enum_types.h @@ -38,12 +38,12 @@ DPCTL_C_EXTERN_C_BEGIN enum DPCTLSyclBackendType { // clang-format off - DPCTL_CUDA = 1 << 13, - DPCTL_HOST = 1 << 14, - DPCTL_LEVEL_ZERO = 1 << 15, - DPCTL_OPENCL = 1 << 16, + DPCTL_CUDA = 1 << 16, + DPCTL_HOST = 1 << 17, + DPCTL_LEVEL_ZERO = 1 << 18, + DPCTL_OPENCL = 1 << 19, DPCTL_UNKNOWN_BACKEND = 0, - DPCTL_ALL_BACKENDS = ((1<<10)-1) << 7 + DPCTL_ALL_BACKENDS = ((1<<5)-1) << 16 // clang-format on }; @@ -57,13 +57,13 @@ enum DPCTLSyclDeviceType // The values should not overlap. // clang-format off - DPCTL_ACCELERATOR = 1 << 1, - DPCTL_AUTOMATIC = 1 << 2, - DPCTL_CPU = 1 << 3, - DPCTL_CUSTOM = 1 << 4, - DPCTL_GPU = 1 << 5, - DPCTL_HOST_DEVICE = 1 << 6, - DPCTL_ALL = (1 << 7) -1 , + DPCTL_ACCELERATOR = 1 << 0, + DPCTL_AUTOMATIC = 1 << 1, + DPCTL_CPU = 1 << 2, + DPCTL_CUSTOM = 1 << 3, + DPCTL_GPU = 1 << 4, + DPCTL_HOST_DEVICE = 1 << 5, + DPCTL_ALL = (1 << 6) - 1, DPCTL_UNKNOWN_DEVICE = 0 // clang-format on };