Implement Negative Tests for clDevice Functions #1148

chemis01 · 2021-02-05T17:56:18Z

Signed-off-by: Chetankumar Mistry chetan.mistry@arm.com

kpet · 2021-02-10T14:11:27Z

test_conformance/api/negative_device.cpp

+    test_failure_error_ret(
+        err, CL_INVALID_PLATFORM,
+        "clGetDeviceIDs should return CL_INVALID_PLATFORM when: \"platform is "
+        "not a valid platform\" using a nullptr",


Passing nullptr for the platform is allowed and the behaviour is implementation-defined:

platform refers to the platform ID returned by clGetPlatformIDs or can be NULL. If platform is
NULL, the behavior is implementation-defined.

Passing nullptr for the platform is allowed and the behaviour is implementation-defined:

platform refers to the platform ID returned by clGetPlatformIDs or can be NULL. If platform is
NULL, the behavior is implementation-defined.

Then we have merged incorrect test case here ? Related with question #1222

Yes, that code is incorrect. I've replied on the issue and asked @chemis01 to prepare a fix.

kpet · 2021-02-10T14:14:03Z

test_conformance/api/negative_device.cpp

+{
+    size_t number_of_properties = 0;
+    int err = clGetDeviceInfo(deviceID, CL_DEVICE_PARTITION_PROPERTIES, 0,
+                              nullptr, &number_of_properties);


It's the total size of the data that's returned, you need to divide by sizeof(cl_device_partition_property) to get the number of properties.

kpet · 2021-02-10T14:15:05Z

test_conformance/api/negative_device.cpp

+    std::vector<cl_device_partition_property> supported_properties(
+        number_of_properties);
+    err = clGetDeviceInfo(deviceID, CL_DEVICE_PARTITION_PROPERTIES,
+                          supported_properties.size(),


supported_properties.size() gives the number of elements, not the total size of the data. You'll need to multiply by sizeof(cl_device_partition_property)

kpet · 2021-02-10T14:15:36Z

test_conformance/api/negative_device.cpp

+    if (supported_properties == SupportedPartitionSchemes::None)
+    {
+        printf("Device does not support creating subdevices... Skipping\n");
+        return TEST_SKIP;


Suggested change

return TEST_SKIP;

return TEST_SKIPPED_ITSELF;

jlewis-austin · 2021-02-11T23:27:28Z

test_conformance/api/negative_device.cpp

+        TEST_FAIL);
+
+    err =
+        clGetDeviceInfo(reinterpret_cast<cl_device_id>(context), CL_DEVICE_TYPE,


We'd been debating these types of tests internally, so I just want to confirm that this level of checking is expected. I thought there had been some suggestion in the past of simple null checks being adequate wrt determining object validity?

This spec issue is related, I think: KhronosGroup/OpenCL-Docs#482

Device IDs are slightly different since you "get" them rather than "create" them and they aren't associated with a context, but there are a lot of similarities to the objects discussed on the issue as well.

Thanks, that's what I was thinking of. I guess our own discussion has broadened to the more generic question of whether error-checking should be required to distinguish between any two resource objects (specifically those implementing the icd cl layout), so it's a question for negative testing on almost every API.

jlewis-austin · 2021-02-11T23:42:12Z

test_conformance/api/negative_device.cpp

+        test_error(err, "clGetPlatformInfo");
+    }
+
+    if (timer_resolution != 0)


Does this work correctly for 2.0 devices?

https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#clGetDeviceAndHostTimer

clGetDeviceAndHostTimer is missing before version 2.1

Thanks, I missed that

jlewis-austin · 2021-02-11T23:58:30Z

test_conformance/api/negative_device.cpp

+        test_error(err, "clGetPlatformInfo");
+    }
+
+    if (timer_resolution != 0)


Same, it seems 2.0 devices will fall in here

test_common/harness/parseParameters.cpp

bashbaug

Mostly LGTM, but can you please check the device partitioning cases?

test_conformance/api/negative_device.cpp

test_conformance/api/negative_platform.cpp

jlewis-austin · 2021-09-22T15:58:41Z

@chemis01 do you mind if I split off the infrastructure required for the #1318 fix into a separate PR (with attribution of course)? Or you can do it if you'd rather. That would also simplify this one a bit and maybe help get it pushed through faster.

chemis01 · 2021-09-22T16:24:08Z

@chemis01 do you mind if I split off the infrastructure required for the #1318 fix into a separate PR (with attribution of course)? Or you can do it if you'd rather. That would also simplify this one a bit and maybe help get it pushed through faster.

@jlewis-austin I am completely fine with you splitting it off into a separate PR.

jlewis-austin · 2022-06-14T16:40:31Z

@chemis01 do you mind if I split off the infrastructure required for the #1318 fix into a separate PR (with attribution of course)? Or you can do it if you'd rather. That would also simplify this one a bit and maybe help get it pushed through faster.

@jlewis-austin I am completely fine with you splitting it off into a separate PR.

We ended up going with a simpler fix in #1374 to expedite it and keep it separate from the refactor work.

Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

This commit addresses upstream comments by: 1) Remove the test for CL_INVALID_PLATFORM when using a nullptr for clGetDeviceIDs 2) Ensure that the number of returned CL_DEVICE_PARTITION_PROPERTIES is handled correctly 3) Replace TEST_SKIP with TEST_SKIPPED_ITSELF Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

Add functionality in the harness to allow negative tests to optionally test Object Validity by passing in a valid object which is not the correct object. This option can be selected by passing --invalid-object-scenarios as a command-line argument. Additionally this change updates the negative platform tests so that they use this feature Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

This change updates the negative device tests so that they can use the optional argument to test for object validity. E.g. optionally use a nullptr or a valid object which is not a device when a devices is needed Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

When testing optional "invalid_object" scenarios, we want to omit the case when a nullptr is used when the function asks for a cl_platform_id. This is because this scenario is implementation defined and so a nullptr is valid. Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

This commit addresses upstream comments by ensuring that CL_DEVICE_PARTITION_BY_COUNTS_LIST_END is present when partioning by Counts Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

This commit addresses upstream comments where the number of out devices and the number of subdevices targeted by the properties variable were creating issues with testing. Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

ahesham-arm · 2024-07-09T10:38:42Z

@kpet Rebased and CI checks are now passing.

bashbaug · 2024-09-30T22:14:42Z

test_conformance/api/negative_device.cpp

+    err = clCreateSubDevices(deviceID, properties, max_for_partition + 1,
+                             out_devices.data(), nullptr);


I think there is still a problem here. On the Intel CPU OpenCL implementation, testing on a CPU with 24 cores, I am seeing this call being generated:

>>>> clCreateSubDevices: in_device = 12th Gen Intel(R) Core(TM) i9-12900K (CL_DEVICE_TYPE_CPU) (0x5e7b00958778), properties = [ CL_DEVICE_PARTITION_EQUALLY = 1 ], num_devices = 25 <<<< clCreateSubDevices -> CL_SUCCESS

This call is returning CL_SUCCESS, and I don't see any reason why it should not return CL_SUCCESS: it is only creating 24 sub-devices with this call and the 25th is un-written, but this should be fine. This certainly is not testing the case where "in_device could not be further partitioned".

Perhaps the properties should be different to test this case? For example, I believe trying to partition equally into a device with 24 compute units will generate the expected error, since in this case the input device is not being further partitioned:

>>>> clCreateSubDevices: in_device = 12th Gen Intel(R) Core(TM) i9-12900K (CL_DEVICE_TYPE_CPU) (0x5f50d1d32778), properties = [ CL_DEVICE_PARTITION_EQUALLY = 24 ], num_devices = 1 ERROR! clCreateSubDevices returned CL_DEVICE_PARTITION_FAILED (-18) <<<< clCreateSubDevices -> CL_DEVICE_PARTITION_FAILED

bashbaug · 2024-09-30T22:19:13Z

test_conformance/api/negative_device.cpp

+        test_failure_error_ret(
+            err, CL_INVALID_DEVICE_PARTITION_COUNT,


Note: our CPU implementation is returning CL_DEVICE_PARTITION_FAILED in this case (and the case below), not CL_INVALID_DEVICE_PARTITION_COUNT. I suppose CL_INVALID_DEVICE_PARTITION_COUNT is more descriptive, but CL_DEVICE_PARTITION_FAILED seems like it is appropriate too. Should we accept both error codes, or do we definitely want this case to return CL_INVALID_DEVICE_PARTITION_COUNT?

kpet requested changes Feb 10, 2021

View reviewed changes

jlewis-austin reviewed Feb 12, 2021

View reviewed changes

chemis01 force-pushed the negative_device_tests branch 4 times, most recently from 2a77710 to 652455a Compare March 25, 2021 09:20

jlewis-austin reviewed Mar 25, 2021

View reviewed changes

test_common/harness/parseParameters.cpp Show resolved Hide resolved

kpet mentioned this pull request Apr 23, 2021

platform can be NULL, when querying platform info by clGetPlatformInfo() #1222

Closed

chemis01 force-pushed the negative_device_tests branch from 652455a to 937bf1f Compare April 30, 2021 10:15

bashbaug requested changes Jun 8, 2021

View reviewed changes

chemis01 force-pushed the negative_device_tests branch from 937bf1f to 5c1aed5 Compare June 11, 2021 11:16

chemis01 force-pushed the negative_device_tests branch 4 times, most recently from 69042f9 to 1a3dc51 Compare July 19, 2021 14:37

kpet mentioned this pull request Sep 7, 2021

Invalid test case for negative_get_platform_info #1318

Closed

jlewis-austin reviewed Sep 7, 2021

View reviewed changes

test_conformance/api/negative_platform.cpp Show resolved Hide resolved

ahesham-arm force-pushed the negative_device_tests branch from 1a3dc51 to d807302 Compare July 8, 2024 21:58

chemis01 added 7 commits July 8, 2024 23:39

Implement Negative Tests for clDevice Functions

b6b8098

Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

[SQUASH] Fix Negative clCreateSubDevices properites

6380505

This commit addresses upstream comments by ensuring that CL_DEVICE_PARTITION_BY_COUNTS_LIST_END is present when partioning by Counts Signed-off-by: Chetankumar Mistry <chetan.mistry@arm.com>

ahesham-arm force-pushed the negative_device_tests branch from d807302 to c89f945 Compare July 8, 2024 22:41

kpet added the focused review label Aug 6, 2024

bashbaug requested changes Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Negative Tests for clDevice Functions #1148

Implement Negative Tests for clDevice Functions #1148

chemis01 commented Feb 5, 2021

kpet Feb 10, 2021

gwawiork Apr 23, 2021

kpet Apr 23, 2021

kpet Feb 10, 2021

kpet Feb 10, 2021

kpet Feb 10, 2021

jlewis-austin Feb 11, 2021

bashbaug Feb 12, 2021

jlewis-austin Feb 12, 2021

jlewis-austin Feb 11, 2021

chemis01 Feb 15, 2021 •

edited

Loading

jlewis-austin Mar 30, 2021

jlewis-austin Feb 11, 2021

bashbaug left a comment

jlewis-austin commented Sep 22, 2021

chemis01 commented Sep 22, 2021

jlewis-austin commented Jun 14, 2022

ahesham-arm commented Jul 9, 2024

bashbaug Sep 30, 2024

bashbaug Sep 30, 2024

		err = clCreateSubDevices(deviceID, properties, max_for_partition + 1,
		out_devices.data(), nullptr);

		test_failure_error_ret(
		err, CL_INVALID_DEVICE_PARTITION_COUNT,

Implement Negative Tests for clDevice Functions #1148

Are you sure you want to change the base?

Implement Negative Tests for clDevice Functions #1148

Conversation

chemis01 commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemis01 Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashbaug left a comment

Choose a reason for hiding this comment

jlewis-austin commented Sep 22, 2021

chemis01 commented Sep 22, 2021

jlewis-austin commented Jun 14, 2022

ahesham-arm commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemis01 Feb 15, 2021 •

edited

Loading