Extend test coverage for `clClonekernel` #1251

jainvikas8 · 2021-05-14T16:22:05Z

Add the following test coverage for the clCloneKernel

With args:
Use clSetKernelArg to set args after kernel is cloned. Enqueue and
read the buffer to validate.

With Execinfo:
Use clSetKernelExecInfo after the kernel is cloned and read the buffer
to validate.

With no args:
Clone a kernel with no args and enqueue.

With SVM pointer:
Use clSetKernelArgSVMPointer to set args after kernel is cloned.
Enqueue and read the buffer to validate.

This fixes #1098 and #1234

This PR also replaces test_error with test_assert_error to report the error during failure.

CLAassistant · 2021-05-14T16:22:14Z

All committers have signed the CLA.

jainvikas8 · 2021-05-17T08:56:48Z

I'm seeing

fatal: unable to access 'https://github.com/KhronosGroup/libclcxx.git/': Could not resolve host: github.com
Command exited with code 128

How can we restart this?

jainvikas8 · 2021-05-17T15:58:53Z

@gwawiork, please review.

mantognini · 2021-05-19T07:40:43Z

I'm seeing

fatal: unable to access 'https://github.com/KhronosGroup/libclcxx.git/': Could not resolve host: github.com
Command exited with code 128

How can we restart this?

I had the same issue on another PR. I ended up re-pushing (on the same branch) to retrigger another run. Hopefully there's a more clever alternative with a simple button to press somewhere, but I couldn't find it.

jainvikas8 · 2021-06-02T13:08:43Z

Forced pushed to rebase.

jainvikas8 · 2021-06-10T15:17:38Z

Forced pushed again, CI was stuck

gwawiork · 2021-08-05T13:57:09Z

Hi @jainvikas8
I've check your changes on Windows on Intel GPU and I am getting
ERROR: clFinish failed! (CL_OUT_OF_RESOURCES from C:\NewVPG\Repo\public-github\opensource-qb\OpenCL-CTS\test_conformance\api\test_clone_kernel.cpp:524) test_exec_enqueue_helper failed for cloneKernel_2 on retry. clCloneKernel test_cloned_kernel_exec_info failed. clone_kernel FAILED

The test process is not able to exit looks like hang. Could you double check on your side if everything is ok ? I will analyze the problem as well.

jainvikas8 · 2021-08-05T15:19:02Z

Forced pushed to rebase.

jainvikas8 · 2021-08-06T10:30:09Z

Hi @jainvikas8
I've check your changes on Windows on Intel GPU and I am getting
ERROR: clFinish failed! (CL_OUT_OF_RESOURCES from C:\NewVPG\Repo\public-github\opensource-qb\OpenCL-CTS\test_conformance\api\test_clone_kernel.cpp:524) test_exec_enqueue_helper failed for cloneKernel_2 on retry. clCloneKernel test_cloned_kernel_exec_info failed. clone_kernel FAILED

The test process is not able to exit looks like hang. Could you double check on your side if everything is ok ? I will analyze the problem as well.

Have not observed this in the past, normally it should fail for this too.
test_exec_enqueue_helper failed for cloneKernel_1 on the retry
Will look into reproducing this issue. Thanks.

gwawiork · 2021-08-09T10:03:43Z

test_conformance/api/test_clone_kernel.cpp

+ sizeof(svmCaps), &svmCaps, NULL);
+ test_error(error, "Unable to query CL_DEVICE_SVM_CAPABILITIES");
+
+ if (svmCaps != 0)


I think we should return TEST_SKIPPED_ITSELF in case svmCaps == 0 now there is TEST_PASS

I think we should return TEST_SKIPPED_ITSELF in case svmCaps == 0 now there is TEST_PASS

Acknowledged

test_conformance/api/test_clone_kernel.cpp

jainvikas8 · 2021-08-09T16:31:39Z

test_conformance/api/test_clone_kernel.cpp

+ cl_command_queue queue, int num_elements)
+{
+ if (test_buff_image_multiple_args(deviceID, context, queue, num_elements)
+ != 0)


Check for TEST_FAIL as test can be

TEST_PASS = 0, TEST_FAIL = 1, TEST_SKIP = 2, TEST_SKIPPED_ITSELF = -100,

jainvikas8 · 2021-08-23T14:40:50Z

Forced pushed to rebase and add 2 new commits to address the review comments.

jainvikas8 · 2021-08-23T14:57:40Z

Hi @jainvikas8
I've check your changes on Windows on Intel GPU and I am getting
ERROR: clFinish failed! (CL_OUT_OF_RESOURCES from C:\NewVPG\Repo\public-github\opensource-qb\OpenCL-CTS\test_conformance\api\test_clone_kernel.cpp:524) test_exec_enqueue_helper failed for cloneKernel_2 on retry. clCloneKernel test_cloned_kernel_exec_info failed. clone_kernel FAILED
The test process is not able to exit looks like hang. Could you double check on your side if everything is ok ? I will analyze the problem as well.

Have not observed this in the past, normally it should fail for this too.
test_exec_enqueue_helper failed for cloneKernel_1 on the retry
Will look into reproducing this issue. Thanks.

@gwawiork I've been unable to reproduce this issue on MALI-GPU in Linux with OpenCL 2.1 and 3.0. So, please let me know your thoughts on this?

ddabek-i · 2021-11-17T13:09:55Z

test_conformance/api/test_clone_kernel.cpp

+ clSVMFree(context, svmPtr_srcKernel_1);
+
+ // enqueue - cloneKernel_1 again, to check if the args were not modified
+ if (test_exec_enqueue_helper(context, queue, pBuf, svmPtr_cloneKernel_1,


From this point on, pBuf still contains pointer to svmPtr_srcKernel_1.
Since test_exec_enqueue_helper does not update the pBuf, the cloneKernel_1 will receive an invalid pointer as argument.

The pointer inside pBuf needs to be updated before enqueuing the kernel.

I have moved clSVMFree to free the buffers in the end.

ddabek-i · 2021-11-17T13:10:59Z

test_conformance/api/test_clone_kernel.cpp

+ error = clFinish(queue);
+ test_error(error, "clFinish failed");


This clFinish should not be needed, since clEnqueueSVMMap is made blocking by passing it CL_TRUE as the second argument.

ddabek-i · 2021-11-17T13:22:03Z

test_conformance/api/test_clone_kernel.cpp

+ "set_kernel_exec_info_kernel");
+ test_error(error, "Unable to create srcKernel");
+
+ BufPtr* pBuf =


It looks like the same structure pBuf will be passed to each kernel (as a pointer) and only the pBuf->store will be updated before enqueuing each kernel. Not sure if this is intended?

@ddabek-i It is indeed intended. We use it to test clSetKernelexecinfo with clCloneKernel

ddabek-i · 2021-11-25T11:43:49Z

@jainvikas8 Please take a look at my review when you have the time.

lakshmih · 2022-05-10T01:55:42Z

[Qualcomm] We tested this and it passed on our 3.0 implementation. Will need more time for a detailed review.

bashbaug

This is a nice start but there are a few nice-to-have fixes and please check that the call to clSetKernelExecInfo is correct.

I also see there is a fair amount of code duplication between the SVM and SVM kernel exec info tests - have you considered any ways to reduce this code duplication?

Thanks!

bashbaug · 2022-05-17T04:54:23Z

test_conformance/api/test_clone_kernel.cpp

-"}\n"
-
- };
+const char* clone_kernel_test_kernel[] = {


Minor: consider using C++11 raw string literals here to make these tests easier to read and modify.

bashbaug · 2022-05-17T05:08:06Z

test_conformance/api/test_clone_kernel.cpp

+ cl_kernel* srcKernel, cl_int* value,
+ cl_mem* bufOut)


Minor: I think we should pass the kernel, the value, and the buffer by value, rather than as a pointer.

bashbaug · 2022-05-17T05:18:21Z

test_conformance/api/test_clone_kernel.cpp

+ error = clSetKernelArgSVMPointer(*srcKernel, 1, pBuf);
+ test_error(error, "clSetKernelArgSVMPointer failed");
+ error = clSetKernelExecInfo(*srcKernel, CL_KERNEL_EXEC_INFO_SVM_PTRS,
+ sizeof(pBuf), pBuf);
+ test_error(error, "clSetKernelExecInfo failed");


This doesn't quite look correct to me. pBuf is already set as an SVM pointer kernel argument, so it does not also need to be passed via clSetKernelExecInfo, but svmPtr_Kernel does. Can you please check that we shouldn't be passing svmPtr_Kernel to clSetKernelExecInfo instead of pBuf?

The call to clSetKernelExecInfo is not needed here, removed.

bashbaug · 2022-05-17T05:19:48Z

test_conformance/api/test_clone_kernel.cpp

+ BufPtr* pBuf, cl_int* svmPtr_Kernel,
+ cl_kernel* srcKernel, cl_int* value)


See above - I think we can pass most (if not all) of these by value.

Do we need to pass pBuf at all?

Minor: I'd also consider renaming this function to something like test_SVM_enqueue_helper, since this helper could work for other SVM tests and not just tests for clSetKernelExecInfo.

srcKernel and value are now passed by value.
pBuf removed as it was not used.
Function renamed to test_svm_enqueue_helper and the other helper removed.

bashbaug · 2022-05-17T05:23:53Z

test_conformance/api/test_clone_kernel.cpp

+int test_svm_enqueue_helper(cl_context context, cl_command_queue queue,
+ cl_int* svmPtr_Kernel, cl_kernel* srcKernel,
+ cl_int* value)


See above - do we need another helper here, or can we reuse the previous helper from clSetKernelExecInfo?

bashbaug · 2022-06-01T20:20:54Z

Removing "focused review" until this PR has an updated owner.

Signed-off-by: John Kesapides <john.kesapides@arm.com>

Use `clSetKernelArg` to set args after kernel is cloned. Enqueue and read the buffer to validate. The test uses `buf_write_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

In test_clone_kernel, if `clEnqueueReadBuffer` was a success then the error code would be `CL_SUCCESS`, which will not print the error message when buffer validation fails, therefore replace with `test_assert_error` to print the error message. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Use `clSetKernelExecInfo` after kernel is cloned and read the buffer to validate. The test uses the `set_kernel_exec_info_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Clone a kernel with no args and enqueue. The test uses `test_kernel_empty` kernel program with no arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Use `clSetKernelArgSVMPointer` to set args after kernel is cloned. Enqueue and read the buffer to validate. The test uses `buf_write_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Review comments

ahesham-arm · 2024-07-16T12:57:29Z

@kpet Rebased and CI checks are passing.
@bashbaug Addressed outstanding review comments.

jainvikas8 force-pushed the extend-clCloneKernel-tests branch from 4b4ddc9 to 2bbf5ec Compare June 2, 2021 13:07

jainvikas8 force-pushed the extend-clCloneKernel-tests branch from 2bbf5ec to 95001a6 Compare June 10, 2021 15:17

jainvikas8 force-pushed the extend-clCloneKernel-tests branch from 95001a6 to a9364f8 Compare August 5, 2021 15:18

gwawiork reviewed Aug 9, 2021

View reviewed changes

jainvikas8 commented Aug 9, 2021

View reviewed changes

test_conformance/api/test_clone_kernel.cpp Show resolved Hide resolved

jainvikas8 commented Aug 9, 2021

View reviewed changes

jainvikas8 force-pushed the extend-clCloneKernel-tests branch from a9364f8 to 40aed32 Compare August 23, 2021 14:39

ddabek-i reviewed Nov 17, 2021

View reviewed changes

jainvikas8 force-pushed the extend-clCloneKernel-tests branch from 40aed32 to 06a81db Compare December 13, 2021 15:33

bashbaug added the focused review label Apr 28, 2022

bashbaug requested changes May 17, 2022

View reviewed changes

bashbaug removed the focused review label Jun 1, 2022

JohnKesapidesARM added a commit to JohnKesapidesARM/OpenCL-CTS that referenced this pull request Oct 17, 2022

Rework PR KhronosGroup#1251 based on review comments

d973b02

Signed-off-by: John Kesapides <john.kesapides@arm.com>

JohnKesapidesARM mentioned this pull request Oct 17, 2022

Clone kernels svm #1550

Open

jainvikas8 added 2 commits July 16, 2024 09:52

Add test coverage for clCloneKernel with args

3d58b8b

Use `clSetKernelArg` to set args after kernel is cloned. Enqueue and read the buffer to validate. The test uses `buf_write_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

jainvikas8 added 4 commits July 16, 2024 09:52

Add test coverage for clCloneKernel with Execinfo

771b05a

Use `clSetKernelExecInfo` after kernel is cloned and read the buffer to validate. The test uses the `set_kernel_exec_info_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Add test covergae for clCloneKernel with no args

2bdcdf7

Clone a kernel with no args and enqueue. The test uses `test_kernel_empty` kernel program with no arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

Add test coverge for clCloneKernel with SVM pointer

791ed1b

Use `clSetKernelArgSVMPointer` to set args after kernel is cloned. Enqueue and read the buffer to validate. The test uses `buf_write_kernel` kernel program with 2 arguments. Signed-off-by: Vikas Katariya <vikas.katariya@arm.com>

fixup! Add test coverage for clCloneKernel with Execinfo

0c2fb2c

Review comments

ahesham-arm force-pushed the extend-clCloneKernel-tests branch from 06a81db to 0bfdac1 Compare July 16, 2024 08:52

fixup! Add test coverge for clCloneKernel with SVM pointer

938be73

Review comments

ahesham-arm force-pushed the extend-clCloneKernel-tests branch from 0bfdac1 to 938be73 Compare July 16, 2024 12:43

		error = clFinish(queue);
		test_error(error, "clFinish failed");

		BufPtr* pBuf, cl_int* svmPtr_Kernel,
		cl_kernel* srcKernel, cl_int* value)

Extend test coverage for clClonekernel #1251

Are you sure you want to change the base?

Extend test coverage for clClonekernel #1251

Conversation

jainvikas8 commented May 14, 2021

CLAassistant commented May 14, 2021 • edited Loading

jainvikas8 commented May 17, 2021

jainvikas8 commented May 17, 2021

mantognini commented May 19, 2021

jainvikas8 commented Jun 2, 2021

jainvikas8 commented Jun 10, 2021

gwawiork commented Aug 5, 2021

jainvikas8 commented Aug 5, 2021

jainvikas8 commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainvikas8 commented Aug 23, 2021

jainvikas8 commented Aug 23, 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

jainvikas8 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

ddabek-i commented Nov 25, 2021

lakshmih commented May 10, 2022

bashbaug left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashbaug commented Jun 1, 2022

ahesham-arm commented Jul 16, 2024

Extend test coverage for `clClonekernel` #1251

Extend test coverage for `clClonekernel` #1251

CLAassistant commented May 14, 2021 •

edited

Loading

jainvikas8 Dec 13, 2021 •

edited

Loading