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

Convert LinearAlgebra::distributed::Vector to Kokkos #14537

Merged
merged 45 commits into from Dec 30, 2022
Merged

Convert LinearAlgebra::distributed::Vector to Kokkos #14537

merged 45 commits into from Dec 30, 2022

Conversation

masterleinad
Copy link
Member

Depends on #14510.

@masterleinad
Copy link
Member Author

With the latest commit, I can run LinearAlgebra::distributed::Vector tests on Intel GPUs.

@masterleinad masterleinad force-pushed the kokkos_la_d_vector_kokkos branch 2 times, most recently from d317012 to f11fc59 Compare December 8, 2022 14:18
@masterleinad
Copy link
Member Author

/rebuild

@tjhei
Copy link
Member

tjhei commented Dec 8, 2022

Can you rebase to see if the new MPI Jenkins passes?

@masterleinad
Copy link
Member Author

Code doesn't compile anymore. Did you mean to push the new commits from Lauren's account?

I fixed the authorship for all commits and reverted unifying the implementations for import_elements. We need the specialization as long as we don't replace TBB with Kokkos which I would prefer to fo somewhere else. All your other comments should be addressed.

@masterleinad
Copy link
Member Author

It seems Kokkos::Experimental::abs didn't exist (but Kokoks::abs does now).

Rombur
Rombur reviewed Dec 29, 2022
View reviewed changes
Copy link
Member

@Rombur Rombur left a comment

Choose a reason for hiding this comment

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

Just minor comments. Do you know if this PR fixes the problem we have with ghosted vector (see the cuda/parallel_vector tests failing here)?

include/deal.II/lac/la_parallel_vector.templates.h Outdated
Kokkos::realloc(indices_dev, tmp_n_elements);
Kokkos::deep_copy(indices_dev,
Kokkos::View<size_type *>(indices.data(),
tmp_n_elements));
Copy link
Member

Choose a reason for hiding this comment

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

indices is already a View so why do you create a new one? Shouldn't this be a subview?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

include/deal.II/lac/la_parallel_vector.templates.h
AssertCuda(error_code);
typename ::dealii::MemorySpace::Default::kokkos_space::execution_space
exec;
Kokkos::parallel_reduce(
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to use a name for all the parallel_reduce

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@masterleinad
Copy link
Member Author

Just minor comments. Do you know if this PR fixes the problem we have with ghosted vector (see the cuda/parallel_vector tests failing here)?

I haven't run the tests on NVIDIA GPUs but only on Intel GPUs (and CPUs using SYCL) and those tests were/are passing there.

@Rombur
Copy link
Member

Rombur commented Dec 29, 2022

Maybe a problem when we use the CUDA-aware MPI path then. Is there something similar to CUDA-aware MPI with SYCL?

@masterleinad
Copy link
Member Author

Yes, there is GPU-aware MPI for Intel GPUs but this pull request doesn't address that. That's what #14571 is for (I have only tested that one with Serial and SYCL+CPU).

@masterleinad masterleinad force-pushed the kokkos_la_d_vector_kokkos branch 4 times, most recently from c9628f1 to cacd8d3 Compare December 29, 2022 19:11
@Rombur Rombur merged commit 25d263f into dealii:master Dec 30, 2022
@kronbichler kronbichler mentioned this pull request Mar 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rombur Rombur Rombur approved these changes

Assignees
No one assigned
Labels
ready for review ready to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@masterleinad @tjhei @Rombur