Extension version checking #2483
Conversation
ahesham-arm
force-pushed
the
ext_version_check
branch
from
74f6474 to
c6b890e
Compare
Function no longer throws an exception when it fails. Instead an error is logged and the function returns `std::nullopt`. Signed-off-by: Ahmed Hesham <ahmed.hesham@arm.com>
Add an overload for `is_extension_available` that additionally checks for the extension version. Change the return type of the existing `is_extension_available` function to `bool` to match what it is actually returning. Signed-off-by: Ahmed Hesham <ahmed.hesham@arm.com>
Change `REQUIRE_EXTENSION` to use the new overload of `is_extension_available`. The macro will now use its argument, `ext`, to construct `ext##_EXTENSION_NAME` and `ext##_EXTENSION_VERSION`, which, for example, correspond to `CL_KHR_SEMAPHORE_EXTENSION_NAME` and `CL_KHR_SEMAPHORE_EXTENSION_VERSION`, respectively. The extension name and version are both defined in the OpenCL headers. This guarantees that the macro is checking for the latest extension version before running the test, and eliminates a class of errors, where the user has a spelling mistake in the extension name to check. This is enforced at compile time, as a variable with a typo in its name will not be defined in the headers. Update all instances of `REQUIRE_EXTENSION` to match. Add a new macro, `HAS_EXTENSION`, that also calls the new overload, but does not check the result or return a test status if the call failed. This is useful for instances where tests require acting upon the result of the call, and not necessarily skip the test. Signed-off-by: Ahmed Hesham <ahmed.hesham@arm.com>
ahesham-arm
force-pushed
the
ext_version_check
branch
from
c6b890e to
eaf07b1
Compare
| * In addition to checking that the extension is in the list of extensions | ||
| * supported by the device, the function will check the extension version. | ||
| * To guarantee compatibility with any breaking changes, the function | ||
| * succeeds only on an exact version match. |
There was a problem hiding this comment.
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:
- Do nothing. 😄
- Rename the function so it's more obvious that it is checking for the exact version, and leave the door open for additional overloads in the future -
is_exact_extension_version_available? - Add more version checking smarts, maybe something like checking for an exact version for major versions less than 1, or a matching major version and a minor version equal to or greater?
Note, we may want to ignore mismatching "patch" versions regardless.
There was a problem hiding this comment.
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.
| constexpr size_t image_dim = 32; | ||
|
|
||
| REQUIRE_EXTENSION("cl_ext_immutable_memory_objects"); | ||
| REQUIRE_EXTENSION(CL_EXT_IMMUTABLE_MEMORY_OBJECTS); |
There was a problem hiding this comment.
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);| if (!is_extension_available(device, ext##_EXTENSION_NAME, \ | ||
| ext##_EXTENSION_VERSION)) \ |
There was a problem hiding this comment.
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.
|
Removing "focused review" for now while discussions are ongoing. |
4 participants
This is a proposal to improve how we check for CL extensions.
The goal is to centralise how we check for extensions and the extension version. The change ensures that, unless otherwise explicitly stated by the test, the device is queried to check for the latest version (as specified in the CL headers) of an extension.
If we agree to this change I can then have follow-up work that uses the new macros everywhere, instead of manual checks.
The pull request is split into commits for easier reviewing, I suggest you review each commit on its own, instead of the pull request as a whole.