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

[HIP][UR] Use primary context in HIP adapter #10514

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Jul 21, 2023

The primary context has been default for a while in CUDA PI/Adapter. See #8197.

This PR brings the HIP adapter up to speed.

It also changes the scoped context to only take a ur_device_handle_t since this is coupled with a native primary context in HIP

@hdelan hdelan requested a review from a team as a code owner July 21, 2023 09:56
@@ -1287,6 +1287,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMPrefetch(
ur_queue_handle_t hQueue, const void *pMem, size_t size,
ur_usm_migration_flags_t flags, uint32_t numEventsInWaitList,
const ur_event_handle_t *phEventWaitList, ur_event_handle_t *phEvent) {
#if HIP_VERSION_MAJOR >= 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also adding this check since hipPointerGetAttribute is not supported in HIP versions < 5

@hdelan hdelan temporarily deployed to aws July 21, 2023 10:19 — with GitHub Actions Inactive
@hdelan
Copy link
Contributor Author

hdelan commented Jul 21, 2023

The unittests would be a good place to test this behaviour but the unittests are broken at the moment, so testing is a TODO

@hdelan hdelan temporarily deployed to aws July 21, 2023 10:57 — with GitHub Actions Inactive
Copy link
Contributor

@jandres742 jandres742 left a comment

Choose a reason for hiding this comment

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

No major comment from my side.

Do we have a list of HIP codeowners/reviewers, or are these files defaulting to L0 reviewers?

@hdelan
Copy link
Contributor Author

hdelan commented Jul 26, 2023

Thanks @jandres742 . I feel like @intel/llvm-reviewers-cuda are probably the most relevant for reviewing HIP adapter stuff

Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

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

LGTM

@hdelan hdelan temporarily deployed to aws July 26, 2023 10:09 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to aws July 26, 2023 10:58 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to aws July 26, 2023 11:36 — with GitHub Actions Inactive
@hdelan
Copy link
Contributor Author

hdelan commented Jul 26, 2023

@intel/llvm-gatekeepers this can be merged

@hdelan
Copy link
Contributor Author

hdelan commented Jul 26, 2023

Also would it be worth changing @intel/llvm-reviewers-cuda to intel/llvm-reviewers-cuda-hip ?

@steffenlarsen
Copy link
Contributor

Technically @intel/dpcpp-l0-pi-reviewers approval is required, but it seems like this has already been considered and that we might want to fix the ownership here. Merging without.

@steffenlarsen steffenlarsen merged commit d1c92cb into intel:sycl Jul 26, 2023
@steffenlarsen
Copy link
Contributor

#10580 should change ownership of the plugin. Whether or not the group should be renamed is something we can discuss further.

@hdelan
Copy link
Contributor Author

hdelan commented Jul 26, 2023

Nice thanks @steffenlarsen !

veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 28, 2023
The primary context has been default for a while in CUDA PI/Adapter. See
intel#8197.

This PR brings the HIP adapter up to speed.

It also changes the scoped context to only take a `ur_device_handle_t`
since this is coupled with a native primary context in HIP
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
The primary context has been default for a while in CUDA PI/Adapter. See
intel#8197.

This PR brings the HIP adapter up to speed.

It also changes the scoped context to only take a `ur_device_handle_t`
since this is coupled with a native primary context in HIP
againull pushed a commit that referenced this pull request Oct 20, 2023
A typo in #10514 got rid of the
extended deleter callbacks. This reinstates them.

---------

Co-authored-by: aelovikov-intel <[email protected]>
Co-authored-by: Kenneth Benzie (Benie) <[email protected]>
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.

4 participants