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.

Sign up for GitHub

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

Jump to bottom

Add the pciAddress of the devices to the GetPlatformAndDeviceName function. #992

Closed
wants to merge 1 commit into from
Closed

Add the pciAddress of the devices to the GetPlatformAndDeviceName function. #992

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion test/conformance/testing/include/uur/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,20 @@ inline std::string GetDeviceName(ur_device_handle_t device) {
}

inline std::string GetPlatformAndDeviceName(ur_device_handle_t device) {
return GetPlatformName(GetPlatform()) + "__" + GetDeviceName(device);
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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@pbalcer pbalcer Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@kbenzie kbenzie Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

pci_address, nullptr);
int j = 0;
for (int i = 0; i < max_address_length; i++) {
if (pci_address[i] != '.' && pci_address[i] != ':') {
id[j] = pci_address[i];
j++;
}
}
return GetPlatformName(GetPlatform()) + "__" + GetDeviceName(device) + id +
"_";
}

ur_result_t GetDeviceType(ur_device_handle_t device,
Expand Down
Loading