Skip to content

[UR] Add extra param to urMemGetNativeHandle #1226

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
Merged
merged 3 commits into from
Jan 31, 2024
Merged

[UR] Add extra param to urMemGetNativeHandle #1226

merged 3 commits into from
Jan 31, 2024

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Jan 4, 2024

Add an extra param to urMemGetNativeHandle as suggested here #1199 (comment)

@hdelan hdelan requested review from a team as code owners January 4, 2024 10:01
@hdelan hdelan requested a review from GeorgeWeb January 4, 2024 10:01
@hdelan hdelan marked this pull request as draft January 4, 2024 10:01
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024 •
edited
Loading

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (40517d2) 15.44% compared to head (fc1f306) 15.44%.

Files Patch % Lines
test/conformance/memory/urMemGetNativeHandle.cpp 0.00% 6 Missing ⚠️
include/ur_print.hpp 0.00% 4 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp 0.00% 3 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp 0.00% 2 Missing ⚠️
source/loader/ur_ldrddi.cpp 0.00% 2 Missing ⚠️
source/adapters/null/ur_nullddi.cpp 0.00% 1 Missing ⚠️
source/loader/ur_libapi.cpp 0.00% 1 Missing ⚠️
...mance/memory/urMemBufferCreateWithNativeHandle.cpp 0.00% 1 Missing ⚠️
...rmance/memory/urMemImageCreateWithNativeHandle.cpp 0.00% 1 Missing ⚠️

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
- Coverage   15.44%   15.44%   -0.01%     
==========================================
  Files         239      239              
  Lines       33961    33970       +9     
  Branches     3757     3758       +1     
==========================================
  Hits         5246     5246              
- Misses      28664    28673       +9     
  Partials       51       51

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

@hdelan hdelan force-pushed the get-native-mem-on-device2 branch 5 times, most recently from 453025b to 2910cd4 Compare January 4, 2024 15:11
@hdelan hdelan marked this pull request as ready for review January 4, 2024 16:01
@hdelan hdelan mentioned this pull request Jan 4, 2024
Merged
@hdelan
Copy link
Contributor Author

hdelan commented Jan 12, 2024

Gentle ping @GeorgeWeb @oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-level-zero-write @oneapi-src/unified-runtime-opencl-write

Would be great to get some reviews

@muhammad-tanvir-1211
Copy link

This PR is blocking the following two PRs on oneMKL as all the buffer tests (with accessor interop) are failing for the rocblas backend with PI_ERROR_INVALID_OPERATION.
uxlfoundation/oneMath#428
uxlfoundation/oneMath#425

Logs:
PR_425.txt
PR_428.txt

GeorgeWeb
GeorgeWeb approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@GeorgeWeb GeorgeWeb left a comment

Choose a reason for hiding this comment

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

HIP and CUDA adapters changes look good to me.

See my inline comment regarding the new return code in the hip-nvidia path of the entry point, which is not a blocker, but I'd like to see your view on it.

This was referenced Jan 12, 2024
Merged
Merged
ldrumm
ldrumm requested changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Your commit messages lack any info into what parameter is added and why. Please include context so future you can actually understand motivation

aarongreig
aarongreig reviewed Jan 12, 2024
View reviewed changes
@hdelan hdelan force-pushed the get-native-mem-on-device2 branch from f88620c to a52551e Compare January 12, 2024 11:47
PietroGhg
PietroGhg approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Native CPU lgtm

@hdelan hdelan force-pushed the get-native-mem-on-device2 branch 2 times, most recently from a63c1e2 to 27f573e Compare January 12, 2024 12:03
@hdelan
Copy link
Contributor Author

hdelan commented Jan 12, 2024 •
edited
Loading

Your commit messages lack any info into what parameter is added and why. Please include context so future you can actually understand motivation

@ldrumm Have changed the commit message

@hdelan hdelan force-pushed the get-native-mem-on-device2 branch from 27f573e to e141d9e Compare January 12, 2024 12:21
@ldrumm
Copy link
Contributor

ldrumm commented Jan 12, 2024

Your commit messages lack any info into what parameter is added and why. Please include context so future you can actually understand motivation

Not any more!

Thanks and LGTM

@hdelan hdelan force-pushed the get-native-mem-on-device2 branch from d78d75f to 5c6b4ba Compare January 22, 2024 21:58
@hdelan
Copy link
Contributor Author

hdelan commented Jan 22, 2024

Friendly ping @oneapi-src/unified-runtime-level-zero-write

nrspruit
nrspruit approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for level-zero

@hdelan hdelan force-pushed the get-native-mem-on-device2 branch from 5c6b4ba to abf316e Compare January 23, 2024 09:10
@hdelan hdelan added ready to merge Added to PR's which are ready to merge and removed ready to merge Added to PR's which are ready to merge labels Jan 23, 2024
@hdelan hdelan force-pushed the get-native-mem-on-device2 branch 2 times, most recently from e4d9bfc to 5efcda5 Compare January 30, 2024 14:34
@hdelan hdelan closed this Jan 30, 2024
@hdelan hdelan reopened this Jan 30, 2024
@hdelan hdelan force-pushed the get-native-mem-on-device2 branch from c76662b to 2357fbc Compare January 30, 2024 17:27
@kbenzie kbenzie merged commit d216eb4 into oneapi-src:main Jan 31, 2024
ldrumm pushed a commit to intel/llvm that referenced this pull request Feb 1, 2024
@hdelan
[SYCL][HIP][CUDA] Use new version of piMemGetNativeHandle and add test (
8427bd2 
#12297)

We want to change the signature of `piMemGetNativeHandle` for reasons
explained here oneapi-src/unified-runtime#1199

Corresponding UR PR:
oneapi-src/unified-runtime#1226

A previous PR added a new entry point
#12199 but it was decided that it is
better to modify the existing entry point
@callumfare callumfare mentioned this pull request Jun 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeWeb GeorgeWeb GeorgeWeb approved these changes

@aarongreig aarongreig aarongreig left review comments

@kbenzie kbenzie kbenzie approved these changes

@ldrumm ldrumm ldrumm approved these changes

@nrspruit nrspruit nrspruit approved these changes

+1 more reviewer

@PietroGhg PietroGhg PietroGhg approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@hdelan @codecov-commenter @muhammad-tanvir-1211 @ldrumm @GeorgeWeb @kbenzie @nrspruit @aarongreig @PietroGhg