Add the pciAddress of the devices to the GetPlatformAndDeviceName function. #992
Conversation
szadam
force-pushed
the
devcloud_devices
branch
from
5cd6bdd
to
834262b
Compare
szadam
force-pushed
the
devcloud_devices
branch
from
834262b
to
4430262
Compare
the GetPlatformAndDeviceName function. This solves a problem with duplicate parameters in gtest when running the test on multiple devices
szadam
force-pushed
the
devcloud_devices
branch
from
4430262
to
d08f844
Compare
| constexpr int max_address_length = 256; | ||
| char pci_address[max_address_length]; | ||
| char id[max_address_length]; | ||
| urDeviceGetInfo(device, UR_DEVICE_INFO_PCI_ADDRESS, sizeof(pci_address), |
There was a problem hiding this comment.
I'm not sure this query can be relied upon to be supported by every adapter. This is also ignoring the returned ur_result_t. I think this should be changed to fallback to setting id to an emprty string when the query fails.
There was a problem hiding this comment.
Hmm, what would happen on OpenCL with CPU runtime when there's multiple numa nodes? I'm guessing that's a situation where we won't get a pcie address and the device name would be identical. So we'd need a cascading set of fallbacks ;)
We just need a stable device identifier just for the gtest process. My first thought was to use the value of the device handle pointer, but, having looked at the implementation, that might change from test to test within the same process.
We could just add an id field next to device handles in all the tests, but that's potentially a lot of changes. But I think I'd prefer that over the alternatives.
Stepping back a little - would it make sense to have a process-unique id for a device in the API? something like "adapter:platform:zyx"?
There was a problem hiding this comment.
Do the ID's need to be externally consistent?
Could we not just use the index of the device in the list returned from urDeviceGet?
There was a problem hiding this comment.
Do the ID's need to be externally consistent?
No, but that would be best so that it's possible to identify e.g., a misbehaving device. All we really need is to make sure that the test name is unique. My first thought was to add a monotonically increasing static number to each device name.
Could we not just use the index of the device in the list returned from
urDeviceGet?
That's what I suggested with my third paragraph. This patch is an attempt at avoiding that (while making it marginally easier to identify the device based on the test name).
There was a problem hiding this comment.
That's what I suggested with my third paragraph. This patch is an attempt at avoiding that (while making it marginally easier to identify the device based on the test name).
Ah sorry, my speedreading is suboptimal 😬
I think this would be the simplest way anyway.
Perhaps to make it easier to link a device index a physical device we could print more identifying information during the Environment::SetUp() step?
|
I have updated the target branch of this PR from the |
|
Already fixed by https://github.com/oneapi-src/unified-runtime/pull/1805/files |
This solves a problem with duplicate parameters in gtest when running the test on multiple devices