Skip to content
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.

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

Implement Negative Command Queue Tests #2010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahesham-arm
Copy link
Contributor

Implement negative tests for the following functions:

  • clCreateCommandQueueWithProperties
  • clCreateCommandQueue
  • clSetDefaultDeviceCommandQueue
  • clRetainCommandQueue
  • clReleaseCommandQueue
  • clGetCommandQueueInfo
  • clSetCommandQueueProperties

@ahesham-arm ahesham-arm force-pushed the negative_command_queue_tests branch 3 times, most recently from 2c71436 to 1e798fa Compare July 11, 2024 09:58
Comment on lines 390 to 395
err = clSetDefaultDeviceCommandQueue(nullptr, deviceID, queue);
test_failure_error_ret(
err, CL_INVALID_CONTEXT,
"clSetDefaultDeviceCommandQueue should return CL_INVALID_CONTEXT when "
"\"context is not a valid context\" using a nullptr",
TEST_FAIL);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a lot of these cases need to be updated to allow for CL_INVALID_OPERATION as well, when the passed-in device ID does not support a replaceable default on-device queue. This can happen if the device is a pre-OpenCL 2.0 device, or if the device is an OpenCL 3.0 device that does not support a replaceable default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return TEST_PASS;
}

int test_negative_set_command_queue_property(cl_device_id deviceID,
Copy link
Contributor

@bashbaug bashbaug Aug 20, 2024

Choose a reason for hiding this comment

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

I think clSetCommandQueueProperty needs to handle the cases when the device is newer than OpenCL 1.0 and hence can return CL_INVALID_OPERATION.

See discussion and the latest changes in the now-merged #1182.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"CL_INVALID_VALUE when: \"values specified in "
"properties are not valid\" using a queue size "
"greater than CL_DEVICE_QUEUE_ON_DEVICE_MAX_SIZE",
TEST_FAIL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Spec:
CL_INVALID_VALUE if values specified in properties are not valid.

CL_INVALID_QUEUE_PROPERTIES if values specified in properties are valid but are not supported by the device.

Looks like this could equally be interpreted as a case for "CL_INVALID_QUEUE_PROPERTIES" since the size itself is "not supported by the device" having exceeded CL_DEVICE_QUEUE_ON_DEVICE_MAX_SIZE.

Would like to discuss this in the next call.

Copy link
Contributor

@lakshmih lakshmih Aug 27, 2024

Choose a reason for hiding this comment

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

Agreed on 8/27 to accept either error code (CL_INVALID_VALUE or CL_INVALID_QUEUE_PROPERTIES) for this case. Arm will make this fix in a subsequent patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahesham-arm Looks like the PR will need updating to address the request above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bashbaug
Copy link
Contributor

Removing the "focused review" label until the comments above have been addressed.

Implement negative tests for the following functions:
    - clCreateCommandQueueWithProperties
    - clCreateCommandQueue
    - clSetDefaultDeviceCommandQueue
    - clRetainCommandQueue
    - clReleaseCommandQueue
    - clGetCommandQueueInfo
    - clSetCommandQueueProperties

Signed-off-by: Chetankumar Mistry <[email protected]>
Signed-off-by: Ahmed Hesham <[email protected]>
"CL_INVALID_VALUE or CL_INVALID_QUEUE_PROPERTIES when: "
"\"values specified in properties are not valid\" using a "
"queue size greather than "
"CL_DEVICE_QUEUE_ON_DEVICE_MAX_SIZE");

Choose a reason for hiding this comment

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

If error occurs, it prints
negative_create_command_queue_with_properties...
clCreateCommandQueueWithProperties should return CL_INVALID_VALUE or CL_INVALID_QUEUE_PROPERTIES when: "values specified in properties are not valid" using a queue size greather than CL_DEVICE_QUEUE_ON_DEVICE_MAX_SIZEnegative_create_command_queue_with_properties FAILED

can we add "\n" or "." to split the 2 sentences? Same comments for other log_error call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants