-
Notifications
You must be signed in to change notification settings - Fork 223
Extension version checking #2483
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,17 +150,22 @@ template <typename T> T *register_test(const char *name, Version version) | |
|
|
||
| #define REGISTER_TEST(name) REGISTER_TEST_VERSION(name, Version(1, 2)) | ||
|
|
||
| #define REQUIRE_EXTENSION(name) \ | ||
| #define REQUIRE_EXTENSION(ext) \ | ||
| do \ | ||
| { \ | ||
| if (!is_extension_available(device, name)) \ | ||
| if (!is_extension_available(device, ext##_EXTENSION_NAME, \ | ||
| ext##_EXTENSION_VERSION)) \ | ||
|
Comment on lines
+156
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be OK, but so we go in with eyes open - I think this means that the tests (that use this macro at least) will only run for implementations that support the extension version in the headers, and only that version. Do we want to have the ability to test other extension versions also? For example, I could envision tests for an extension that have one codepath for version 1.X and another codepath for version 2.X, kind of like the tests for integer dot product, but perhaps with a breaking change between 1.x and 2.X. For usages like this one I don't think we'd be able to use this REQUIRE_EXTENSION macro the way it is currently defined. |
||
| { \ | ||
| log_info(name \ | ||
| log_info(ext##_EXTENSION_NAME \ | ||
| " is not supported on this device. Skipping test.\n"); \ | ||
| return TEST_SKIPPED_ITSELF; \ | ||
| } \ | ||
| } while (0) | ||
|
|
||
| #define HAS_EXTENSION(ext) \ | ||
| is_extension_available(device, ext##_EXTENSION_NAME, \ | ||
| ext##_EXTENSION_VERSION) | ||
|
|
||
| extern int gFailCount; | ||
| extern int gTestCount; | ||
| extern cl_uint gReSeed; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ REGISTER_TEST(negative_enqueue_map_image) | |
| { | ||
| constexpr size_t image_dim = 32; | ||
|
|
||
| REQUIRE_EXTENSION("cl_ext_immutable_memory_objects"); | ||
| REQUIRE_EXTENSION(CL_EXT_IMMUTABLE_MEMORY_OBJECTS); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is clever, but I wonder if it's a bit too clever, since things like intellisense linking and even grep'ing for these symbols won't work. Should we just type out the extension name and extension version instead? REQUIRE_EXTENSION(
CL_EXT_IMMUTABLE_MEMORY_OBJECTS_EXTENSION_NAME,
CL_EXT_IMMUTABLE_MEMORY_OBJECTS_VERSION); |
||
|
|
||
| static constexpr cl_mem_flags mem_flags[]{ | ||
| CL_MEM_IMMUTABLE_EXT | CL_MEM_USE_HOST_PTR, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand the rationale for checking for an exact version match, but I also think this is likely to cause confusion and problems in the future. Couple of options to consider:
is_exact_extension_version_available?Note, we may want to ignore mismatching "patch" versions regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything to add, but this thread made me think of this CTS Issue we still have #2152 open about how the versions of provisional extensions is tested. It would be nice if we could resolve that issue as part of this PR.