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

Strengthen testing of CL_DEVICE_TYPE query and use of CL_DEVICE_TYPE* #1977

Merged
merged 7 commits into from
Aug 20, 2024

Conversation

kamil-goras-mobica
Copy link
Contributor

Issue: #1930

total_errors++;
}
test_assert_error(num_devices > 1,
"clGetDeviceIDs must return at least one device");
Copy link
Contributor

Choose a reason for hiding this comment

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

Condition should either 'num_devices >= 1' or 'num_devices > 0' and not 'num_devices > 1'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lakshmih this will be changed to ">=1"

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 both the log_error and the test_assert_error? Seems redundant. Can we remove the test_assert_error case?

Copy link
Contributor

@lakshmih lakshmih left a comment

Choose a reason for hiding this comment

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

Need to fix the conditional check in platform.cpp

@@ -89,6 +89,8 @@ extern int test_min_max_language_version(cl_device_id deviceID, cl_contex
extern int test_native_kernel(cl_device_id device, cl_context context, cl_command_queue queue, int n_elems );

extern int test_create_context_from_type(cl_device_id deviceID, cl_context context, cl_command_queue queue, int num_elements);
extern int test_create_context_from_type_device_type_all(cl_device_id deviceID, cl_context context, cl_command_queue queue, int num_elements);
extern int test_create_context_from_type_device_type_default(cl_device_id deviceID, cl_context context, cl_command_queue queue, int num_elements);
Copy link
Contributor

Choose a reason for hiding this comment

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

context_from_type goes beyond just creating the context by creating memory objects and enqueueing a kernel etc.
Should these other tests do that as welll?
num_elements seems to be unused in all cases and could be removed.

Copy link
Contributor Author

@kamil-goras-mobica kamil-goras-mobica Jul 11, 2024

Choose a reason for hiding this comment

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

@lakshmih No, I just wanted to create test scenarios with clCreateContextFromType with TYPE_ALL and TYPE_DEFAULT. I did not find any sufficient place so I put it here.
num_elements is needed by ADD_TEST macro

test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
test_conformance/api/test_create_context_from_type.cpp Outdated Show resolved Hide resolved
total_errors++;
}
test_assert_error(num_devices > 1,
"clGetDeviceIDs must return at least one device");
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 both the log_error and the test_assert_error? Seems redundant. Can we remove the test_assert_error case?

test_conformance/api/test_platform.cpp Outdated Show resolved Hide resolved
test_conformance/device_execution/device_info.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@bashbaug
Copy link
Contributor

Merging as discussed in the August 20th teleconference.

@bashbaug bashbaug merged commit 746544a into KhronosGroup:main Aug 20, 2024
7 checks passed
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.

4 participants