Skip to content

[draft] Add new experimental entry point for get native mem on device#1199

Closed
hdelan wants to merge 1 commit intooneapi-src:mainfrom
hdelan:get-native-mem-on-device
Closed

[draft] Add new experimental entry point for get native mem on device#1199
hdelan wants to merge 1 commit intooneapi-src:mainfrom
hdelan:get-native-mem-on-device

Conversation

@hdelan
Copy link
Contributor

@hdelan hdelan commented Dec 18, 2023

Accessor interop requires urMemGetNativeHandle to return a valid mem handle. Multi dev ctx breaks this because a UR mem can be on many different devices now, so we need a new entry point that also takes a device or a queue param.

Will wait until @kbenzie is back to discuss the best approach

@hdelan hdelan requested review from a team as code owners December 18, 2023 11:45
@hdelan hdelan requested a review from mmoadeli December 18, 2023 11:45
@hdelan hdelan force-pushed the get-native-mem-on-device branch 3 times, most recently from 358e0bc to 85ae4b0 Compare December 18, 2023 13:17
@hdelan hdelan force-pushed the get-native-mem-on-device branch from 85ae4b0 to e4682d9 Compare December 18, 2023 15:00
@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (8d1486a) 15.74% compared to head (e4682d9) 15.82%.

Files Patch % Lines
include/ur_print.hpp 0.00% 18 Missing ⚠️
source/loader/ur_ldrddi.cpp 48.38% 16 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp 46.42% 15 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp 54.16% 11 Missing ⚠️
source/adapters/null/ur_nullddi.cpp 37.50% 10 Missing ⚠️
source/loader/ur_libapi.cpp 0.00% 7 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1199      +/-   ##
==========================================
+ Coverage   15.74%   15.82%   +0.08%     
==========================================
  Files         223      223              
  Lines       31466    31591     +125     
  Branches     3556     3578      +22     
==========================================
+ Hits         4953     5000      +47     
- Misses      26462    26540      +78     
  Partials       51       51              

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

Comment on lines +459 to +462
- type: $x_device_handle_t
name: hDevice
desc: |
[in] handle of the device that native handle will be resident on.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just modify the existing GetNativeHandle to add this argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that not an ABI breaking change that will take some time to get merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't claim ABI stability in UR at this time, this won't be an issue as long as the necessary SYCL RT changes are made in tandem with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thanks @kbenzie . I will close this PR and open a new one with just changes to the existing entry point

@hdelan
Copy link
Contributor Author

hdelan commented Jan 4, 2024

Hi @kbenzie this entry point is needed for both accessor interop as well as buffer interop. So do you think it's OK to have the same entry point for both (as is currently the case)? For accessor interop we can call urMemGetNativeHandle(Mem, Dev, &Handle) and for the buffer interop we just call the same but with the Dev arg as nullptr?

Do you think that is acceptable or should we split this into two entry points, as this PR does?

@kbenzie
Copy link
Contributor

kbenzie commented Jan 4, 2024

I'm not really sure what the implications are here, would need to have a bit more info about why the buffer interop path differs I think.

ldrumm pushed a commit to intel/llvm that referenced this pull request Feb 1, 2024
#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants