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

Move HIP out of Experimental #5383

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented Aug 24, 2022

Move the execution and memory spaces associated with HIP outside of experimental. I have added an alias in core/src/Kokkos_HIP.hpp so that current codes still work. The old code is not deprecated, I wasn't sure what was the plan.

namespace Experimental {
class SYCL;
class HIP;
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is not invalid. The compiler compiles that Experimental::HIP is already defined. I don't think that many users will hit that problem and the fix in user code is pretty simple.

Copy link
Member

Choose a reason for hiding this comment

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

not -> now

In other words, this PR would break code that had forward declarations of HIP or HIPSpace in Kokkos::Experimental namespace.

@dalg24
Copy link
Member

dalg24 commented Aug 25, 2022

Retest this please

Copy link
Contributor

@JBludau JBludau 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.
The only two things I found that still use Experimental::HIP are in:
./core/unit_test/TestAbort.hpp:123: if (std::is_same<ExecutionSpace, Kokkos::Experimental::HIP>::value) {
./core/unit_test/TestViewMemoryAccessViolation.hpp:202: if (std::is_same<ExecutionSpace, Kokkos::Experimental::HIP>::value) {

std::string("duplicated_") + original_view.label(),
exec_space),
arg_N[0], arg_N[1], arg_N[2], arg_N[3], arg_N[4], arg_N[5], arg_N[6],
arg_N[7]);
Copy link
Member

Choose a reason for hiding this comment

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

so weird ....

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the wrong version of clang-format. Will fix it.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Yeah lets get those two guys still fixed and then we can merge this.

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.

None yet

4 participants