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.

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

[EXP][usm p2p] Add test for ur_exp_usm_p2p and impl for hip #1369

Merged
merged 16 commits into from
Mar 18, 2024

Conversation

JackAKirk
Copy link
Contributor

@JackAKirk JackAKirk commented Feb 21, 2024

  • Adds USM_P2P impl for hip backend.
  • Introduces UR_USM_P2P_EXTENSION_STRING_EXP macro to the spec which is used to check for usm_p2p experimental extension support in UR.
  • The existing supported backend for this extension (CUDA) now correctly returns this device value string.
  • This device value string is used in the new test that is introduced to check the basic extension behavior for all implementations.

This PR matches the precedent set in #1089 for experimental extension conformance testing.

Uses the latest non deprecated hip APIs.

fully tested and passing via DPC++ using Crusher.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
These changes will be made in separate PRs.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 12.48%. Comparing base (78ef1ca) to head (ad198f9).
Report is 153 commits behind head on main.

Files Patch % Lines
test/conformance/exp_usm_p2p/usm_p2p.cpp 0.00% 20 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1369      +/-   ##
==========================================
- Coverage   14.82%   12.48%   -2.34%     
==========================================
  Files         250      240      -10     
  Lines       36220    36023     -197     
  Branches     4094     4086       -8     
==========================================
- Hits         5369     4498     -871     
- Misses      30800    31521     +721     
+ Partials       51        4      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

source/adapters/cuda/device.cpp Show resolved Hide resolved
include/ur_api.h Outdated
@@ -8885,6 +8885,13 @@ urUSMReleaseExp(
#if !defined(__GNUC__)
#pragma region usm p2p(experimental)
#endif
///////////////////////////////////////////////////////////////////////////////
#ifndef UR_USM_P2P_EXTENSION_STRING_EXP
/// @brief The extension string which defines support for USM P2P which is
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd replace one of the twowhichs with that.

This experimental extension to the Unified-Runtime API aims to provide a
portable interface that can call appropriate driver functions to query and
control peer memory access within different adapters such as CUDA, HIP and
Level Zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor change to above that you may simply ignore

Programming models like SYCL or OpenMP aim to support several important projects that utilise fine-grained peer-to-peer memory access controls. This experimental extension to the Unified-Runtime API aims to offer a portable interface capable of invoking relevant driver functions to query about and control peer memory access across various adapters, including CUDA, HIP, and Level Zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll switch the first sentence, I think this is an improvement. I think the second one is not as good as the original though.

nullptr));

std::string_view extensions_string(returned_extensions.get());
bool usm_p2p_support =
Copy link
Contributor

Choose a reason for hiding this comment

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

const bool usm_p2p_support

+===========+=============================================+
| 1.0 | Initial Draft |
+-----------+---------------------------------------------+
| 1.1 | Added USM_P2P_EXTENSION_STRING_EXP ID Macro |
Copy link
Contributor

@mmoadeli mmoadeli Feb 23, 2024

Choose a reason for hiding this comment

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

not sure if that should be revision 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following other extensions that iterate through 1.1,1.2 etc. This is a minor change to the spec.

@JackAKirk JackAKirk added the ready to merge Added to PR's which are ready to merge label Feb 23, 2024
@kbenzie kbenzie added the v0.9.x Include in the v0.9.x release label Mar 11, 2024
@kbenzie
Copy link
Contributor

kbenzie commented Mar 11, 2024

Please merge in the main branch so we have up to date test runs, it should also have an intel/llvm PR for testing.

@JackAKirk
Copy link
Contributor Author

Please merge in the main branch so we have up to date test runs, it should also have an intel/llvm PR for testing.

I didn't add a intel/llvm PR for testing here because it doesn't affect any intel/llvm code at all. The only code anywhere that this uses is in the test that is added.
I can merge ur if you want but FYI the situation here is basically the same: no existing tests are affected by the code change.

Allow device extension queries for USM P2P support.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk requested a review from a team as a code owner March 15, 2024 09:56
@JackAKirk JackAKirk changed the title [EXP][usm p2p] Add device extension value string and test for ur_exp_usm_p2p [EXP][usm p2p] Add test for ur_exp_usm_p2p and impl for hip Mar 15, 2024
@kbenzie kbenzie merged commit 94af6e3 into oneapi-src:main Mar 18, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge v0.9.x Include in the v0.9.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants