-
Notifications
You must be signed in to change notification settings - Fork 111
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
[hip] Remove deprecated hip APIs, simplify urContext #1830
base: main
Are you sure you want to change the base?
Conversation
intel/llvm tested with these changes here: intel/llvm#14476 |
source/adapters/hip/device.hpp
Outdated
namespace { | ||
/// Scoped Device is used across all UR HIP plugin implementation to activate | ||
/// the native Device on the current thread. The ScopedDevice does not | ||
/// reinstate the previous device as all operations in the hip adapter that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was copied from an older comment but we have HIP
and hip
in the same comment. I think HIP
is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've used HIP consistently now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
command-buffer change LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few minor questions.
source/adapters/hip/context.hpp
Outdated
std::vector<ur_device_handle_t> Devices; | ||
|
||
std::atomic_uint32_t RefCount; | ||
|
||
ur_context_handle_t_(const ur_device_handle_t *Devs, uint32_t NumDevices) | ||
: Devices{Devs, Devs + NumDevices}, RefCount{1} { | ||
for (auto &Dev : Devices) { | ||
urDeviceRetain(Dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no longer retaining ur devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it back.
if (!hDevice) { | ||
throw UR_RESULT_ERROR_INVALID_DEVICE; | ||
} | ||
UR_CHECK_ERROR(hipSetDevice(hDevice->getIndex())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
Mostly these APIs were completely redundant already. HIP amd doesn't have have a context concept. Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
hip -> HIP in comment. 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>
The test should have been checking for success or unsupported, since most backends don't support context interop. Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk Please make sure intel/llvm#14476 is updated and passing CI, so we could merge this. Thanks! |
I've removed ready to merge since the intel/llvm PR isn't passing testing and is still awaiting approvals. Once these are in place, add the ready to merge label again. |
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
This updates a context test to pass if the interface is unsupported, which cleans up the native_cpu match file that doesn't support certain context interfaces. |
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Thanks, tests for intel/llvm#14476 pass now and has approval, so I've put back ready to merge. I'll update that PR with the commit hash once this PR is merged. |
hip amd does not have a native context type other than to support the cuContext type on Nvidia devices. hip plans to completely remove usage of this type anyway, with all corresponding APIs marked deprecated.
This PR future-proofs our hip implementation for when these deprecated APIs become unsupported. It also simplifies the urContext type.
This updates a context test to pass if the interface is unsupported, which cleans up the native_cpu match file that doesn't support certain context interfaces.