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][CUDA] Change unions in ur_mem_handle_t_ to stdvariant #961

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Oct 13, 2023

Std::variant is safer than unions in the ur_mem_handle_t_ class and it will make it easier to incrementally move to multi device ctx, which requires adding std::vectors to the MemImpl, which is not possible in a union.

@hdelan hdelan requested review from a team as code owners October 13, 2023 15:48
@hdelan
Copy link
Contributor Author

hdelan commented Oct 13, 2023

intel/llvm testing: intel/llvm#11539

@hdelan hdelan requested a review from jchlanda October 17, 2023 10:55
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, sorry about the wait.

source/adapters/cuda/memory.cpp Show resolved Hide resolved
source/adapters/cuda/memory.hpp Show resolved Hide resolved
source/adapters/cuda/memory.hpp Show resolved Hide resolved
@hdelan
Copy link
Contributor Author

hdelan commented Oct 18, 2023

@kbenzie can we merge this please?

@kbenzie
Copy link
Contributor

kbenzie commented Oct 18, 2023

@kbenzie can we merge this please?

We already have 2 UR changes that can't merge to intel/llvm because of test failures, I feel like merging this will only add to that problem.

@hdelan
Copy link
Contributor Author

hdelan commented Oct 18, 2023

@kbenzie OK sure yes no rush

@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Oct 19, 2023
@hdelan hdelan force-pushed the change-unions-to-stdvariant branch from 2b2e234 to 86f96f0 Compare October 24, 2023 14:50
@kbenzie kbenzie merged commit 036b9cf into oneapi-src:adapters Oct 27, 2023
48 checks passed
EwanC added a commit to Bensuo/unified-runtime that referenced this pull request Oct 30, 2023
This is required after oneapi-src#961
againull pushed a commit to intel/llvm that referenced this pull request Oct 30, 2023
Update UR SHA to oneapi-src/unified-runtime#961
change unions to std::variant in the HIP and CUDA adapters.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
maarquitos14 pushed a commit to intel/llvm that referenced this pull request Oct 31, 2023
Update UR SHA to oneapi-src/unified-runtime#961
change unions to std::variant in the HIP and CUDA adapters.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants