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.

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

MC/ROCM: ROCm compatibility fixes #858

Merged
merged 1 commit into from
Oct 19, 2023
Merged

MC/ROCM: ROCm compatibility fixes #858

merged 1 commit into from
Oct 19, 2023

Conversation

nileshnegi
Copy link
Contributor

What

  • Adds condition based on HIP_VERSION for using type instead of memoryType in hipPointerAttribute_t.
  • Adds switch case for hipMemoryTypeManaged

Why ?

  • From ROCm 5.5 onwards, memoryType is deprecated and type is used instead in hipPointerAttribute_t.
  • From ROCm 5.7.1 onwards, hipMallocManaged pointers return hipMemoryTypeManaged instead of hipMemoryTypeHost.

@swx-jenkins3
Copy link

Can one of the admins verify this patch?

@nileshnegi nileshnegi force-pushed the topic/hip-ptr-attr-type-fix branch from 2968806 to 1c99c0d Compare October 16, 2023 21:34
@nileshnegi nileshnegi changed the title MC/ROCM: Add HIP_VERSION condition for ROCm compatibility MC/ROCM: add HIP_VERSION condition for ROCm compatibility Oct 16, 2023
@nileshnegi nileshnegi force-pushed the topic/hip-ptr-attr-type-fix branch from 1c99c0d to 6fa90b0 Compare October 16, 2023 21:38
@nileshnegi nileshnegi changed the title MC/ROCM: add HIP_VERSION condition for ROCm compatibility MC/ROCM: ROCm compatibility fixes Oct 16, 2023
Sergei-Lebedev
Sergei-Lebedev approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@Sergei-Lebedev Sergei-Lebedev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Regarding managed memory support, I think proper memory allocation function is needed to support ROCM_MANAGED for algorithms with scratch buffer e.g. allreduce in TL/UCP

@edgargabriel
Copy link
Contributor

Looks good to me. Regarding managed memory support, I think proper memory allocation function is needed to support ROCM_MANAGED for algorithms with scratch buffer e.g. allreduce in TL/UCP

@Sergei-Lebedev thank you, yes, you are correct, it is something that we need to add to our to-do list for the near future.

@Sergei-Lebedev Sergei-Lebedev force-pushed the topic/hip-ptr-attr-type-fix branch from 6fa90b0 to ba93b26 Compare October 19, 2023 16:12
@Sergei-Lebedev Sergei-Lebedev enabled auto-merge (squash) October 19, 2023 16:13
@Sergei-Lebedev Sergei-Lebedev merged commit 6071d2c into openucx:master Oct 19, 2023
7 of 9 checks passed
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
@nileshnegi @nsarka
MC/ROCM: ROCm compatibility fixes (openucx#858)
22658cb 
Signed-off-by: nileshnegi <[email protected]>
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
@nileshnegi @nsarka
MC/ROCM: ROCm compatibility fixes (openucx#858)
6900361 
Signed-off-by: nileshnegi <[email protected]>
janjust pushed a commit to janjust/ucc that referenced this pull request Jan 31, 2024
@nileshnegi @janjust
MC/ROCM: ROCm compatibility fixes (openucx#858)
69eefa1 
Signed-off-by: nileshnegi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samnordmann samnordmann samnordmann approved these changes

@Sergei-Lebedev Sergei-Lebedev Sergei-Lebedev approved these changes

Assignees
No one assigned
Labels
Ready-for-Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nileshnegi @swx-jenkins3 @edgargabriel @Sergei-Lebedev @samnordmann