[UR] fix: Make NODE_MASK and LUID queries return correct status in CUDA and Level Zero - #22574
[UR] fix: Make NODE_MASK and LUID queries return correct status in CUDA and Level Zero#22574mianowski wants to merge 24 commits into
Conversation
mianowski
changed the title
…ero to conform with ur_api
e7da01e to
0c77468
Compare
mianowski
changed the title
mianowski
changed the title
mianowski
changed the title
…hub.com/mianowski/llvm into fix/URT-1194-node-mask-luid-unsupported
… tests to original state
There was a problem hiding this comment.
Pull request overview
Fixes Unified Runtime urDeviceGetInfo behavior for UR_DEVICE_INFO_LUID and UR_DEVICE_INFO_NODE_MASK so that unsupported backends/platforms return UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION (as expected for optional queries), and adds conformance coverage to prevent regressions.
Changes:
- Add conformance tests for
UR_DEVICE_INFO_LUIDandUR_DEVICE_INFO_NODE_MASKthat enforce the correct “optional query” status behavior. - Update CUDA and Level Zero adapters to return
UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION(including when the underlying API reports “unsupported” or returns unusable zero values). - Fix OpenCL node mask query type/size usage (
cl_uintinstead ofcl_int) to match the OpenCL property type.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| unified-runtime/test/conformance/device/urDeviceGetInfo.cpp | Adds optional-query conformance tests for LUID and node mask (and validates non-zero LUID when supported). |
| unified-runtime/source/adapters/opencl/device.cpp | Corrects node mask storage type and queried size to cl_uint. |
| unified-runtime/source/adapters/native_cpu/device.cpp | Explicitly marks LUID and node mask as unsupported enumerations for this adapter. |
| unified-runtime/source/adapters/level_zero/device.cpp | Maps unsupported/unusable LUID + node mask to UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION with explicit ZE error handling. |
| unified-runtime/source/adapters/cuda/device.cpp | Maps CUDA “not supported” and unusable zero LUID/node mask cases to UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION. |
| ASSERT_EQ(property_size, sizeof(uint32_t)); | ||
|
|
||
| uint32_t property_value = 0; | ||
| ASSERT_QUERY_RETURNS_VALUE(urDeviceGetInfo(device, property_name, |
There was a problem hiding this comment.
Is there a need to verify that a second call to urDeviceGetInfo results in the same value stored in the 'property_value' variable?
Perhaps it would be a good idea to put more emphasis on checking the validity of the value stored in the 'property_value' variable? Is there a valid range of values that the variable may contain?
There was a problem hiding this comment.
Removed. (It was here for conventional purposes of checking stability between calls but it seems not needed for static hardware properties of NODE_MASK and LUID)
| property_name); | ||
| ASSERT_EQ(property_size, sizeof(std::array<unsigned char, 8>)); | ||
|
|
||
| std::array<unsigned char, 8> property_value{}; |
There was a problem hiding this comment.
Consider using a descriptive name, rather than a magic number (8), especially since it is used more than once (see line 2694).
|
@intel/llvm-gatekeepers please consider merging |
|
can the PR title be updated to match LLVM guidelines? |
|
@intel/llvm-gatekeepers please consider merging |
|
Needs to be rebased and PR title seems not updated |
mianowski
changed the title
…eded for static hardware properties.
|
|
||
| uint64_t luid_as_uint64 = 0; | ||
| std::memcpy(&luid_as_uint64, property_value.data(), sizeof(luid_as_uint64)); | ||
| const bool is_power_of_two = |
There was a problem hiding this comment.
This validation is incorrect. LUID (Locally Unique Identifier) is not required to be a power of 2. LUID is an 8-byte device identifier that can be any non-zero value. LUID should only be checked for all-zeros (which indicates an error).
| uint32_t property_value = 0; | ||
| ASSERT_SUCCESS(urDeviceGetInfo(device, property_name, property_size, | ||
| &property_value, nullptr)); | ||
| } |
There was a problem hiding this comment.
The test doesn't validate the nodeMask value. nodeMask == 0 means unsupported (should return UNSUPPORTED_ENUMERATION).
The NODE_MASK and LUID queries return an incorrect status in CUDA - they return UNSUPORTED_FEATURE instead of UR_RESULT_ERROR_UNSUPORTED_ENUMERATION. This issue was not detected by any tests.