Skip to content

[SYCL][CUDA] Support device ID and UUID in the CUDA Plugin #8203

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Feb 9, 2023
Merged

[SYCL][CUDA] Support device ID and UUID in the CUDA Plugin #8203

merged 2 commits into from
Feb 9, 2023

Conversation

zjin-lcf
Copy link
Contributor

@zjin-lcf zjin-lcf commented Feb 3, 2023

This PR tries to enable the accesses to device ID and UUID with the CUDA driver APIs. Thank you for your review.

Reference
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_intel_device_info.md

@zjin-lcf zjin-lcf requested a review from a team as a code owner February 3, 2023 23:05
@zjin-lcf zjin-lcf requested a review from npmiller February 3, 2023 23:05
@zjin-lcf
Copy link
Contributor Author

zjin-lcf commented Feb 6, 2023

The code changes reported by lint are not consistent with the coding style in the plugin.

@abagusetty
Copy link
Contributor

The code changes reported by lint are not consistent with the coding style in the plugin.

I believe the CI uses clang-format-v13 for formatting. Were you using anything other versions ?

@zjin-lcf
Copy link
Contributor Author

zjin-lcf commented Feb 6, 2023

No, I just looked at other codes in the plugin, and then follow that style.

@npmiller
Copy link
Contributor

npmiller commented Feb 6, 2023

The linter is consistent, there's other parts of the plugins that are formatted like it is suggesting here, and all of the plugin code has gone through the linter.

It's just that the rules it uses to format are not always directly obvious, for example for this:

     sycl::detail::pi::assertion(
-        cuDeviceGetAttribute(&value,
-                             CU_DEVICE_ATTRIBUTE_PCI_DEVICE_ID,
+        cuDeviceGetAttribute(&value, CU_DEVICE_ATTRIBUTE_PCI_DEVICE_ID,
                              device->get()) == CUDA_SUCCESS);

If you look at other places that have it like this, they have enums with longer names that wouldn't fit on the first line in 80 columns.

Line breaking rules can get very tricky in C++, and that's why we should trust the linter even if it's not always the prettiest.

So could you update the code as the linter is suggesting? And in general I suggest using clang-format, you can even build it as part of DPC++ ninja clang-format.

Other than that the patch looks good to me 👍


case PI_DEVICE_INFO_UUID: {
int driver_version = 0;
cuDriverGetVersion(&driver_version);
Copy link
Contributor

Choose a reason for hiding this comment

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

@zjin-lcf @npmiller Do we need a API check for this. Also would you suggest to replace sycl::detail::pi::assertion to PI_CHECK_ERROR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the CUDA docs it looks like this can only return an error if the parameter is nullptr, so I think this should be fine, see:

@zjin-lcf zjin-lcf changed the title [SYCL][CUDA] support device ID and UUID in the CUDA Plugin [SYCL][CUDA] Support device ID and UUID in the CUDA Plugin Feb 6, 2023
@zjin-lcf zjin-lcf temporarily deployed to aws February 6, 2023 17:33 — with GitHub Actions Inactive
@zjin-lcf zjin-lcf temporarily deployed to aws February 6, 2023 18:32 — with GitHub Actions Inactive
@bader bader requested a review from abagusetty February 7, 2023 01:21
Copy link
Contributor

@abagusetty abagusetty left a comment

Choose a reason for hiding this comment

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

LGTM!

@bader bader merged commit 8213074 into intel:sycl Feb 9, 2023
@bader
Copy link
Contributor

bader commented Feb 9, 2023

@zjin-lcf, thanks for contributing this!

@zjin-lcf
Copy link
Contributor Author

zjin-lcf commented Feb 9, 2023

I want to thank the reviewers for their comments.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants