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

Use Kokkos in functions struct replacing TBB #14633

Conversation

masterleinad
Copy link
Member

Currently, we are using TBB for most vector operations on the host and Kokkos for the Default memory space. This pull request unifies the implementation by always using Kokkos. We should evaluate if the performance on the host is acceptable (if Kokkos was configured with a parallel host execution space).

@tamiko
Copy link
Member

tamiko commented Jan 3, 2023

@masterleinad @kronbichler What would be a good setup to test this? I am happy to run some extended performance tests.

@tjhei
Copy link
Member

tjhei commented Jan 3, 2023

if Kokkos was configured with a parallel host execution space).

If I configure with Kokkos_ENABLE_THREADS or Kokkos_ENABLE_OPENMP, right? Any preference between them or anything else we need to know?

It would also be sensible to check if we see no overhead for an MPI-only run where one would not want vector operations to be threaded...

@Rombur
Copy link
Member

Rombur commented Jan 3, 2023

If I configure with Kokkos_ENABLE_THREADS or Kokkos_ENABLE_OPENMP, right? Any preference between them or anything else we need to know?

Probably better to use OpenMP over threads. You want to set OMP_PROC_BIND=spread and OMP_PLACES=threads to get better performance.

@masterleinad
Copy link
Member Author

@masterleinad @kronbichler What would be a good setup to test this? I am happy to run some extended performance tests.

The benchmark in #2496 (comment) using LinearAlgebra::distributed::Vector would likely be suitable. Up to now, LinearAlgebra::Vector is not affected by this pull request.

@masterleinad
Copy link
Member Author

/rebuild

Comment on lines +121 to +123
Kokkos::View<T *, typename MemorySpace::kokkos_space> new_values)
: values_host_buffer(Kokkos::View<T *, Kokkos::HostSpace>("host buffer", 0))
, values(new_values)
Copy link
Member

Choose a reason for hiding this comment

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

You're just copying new_values twice this way. Make it a const reference to avoid one of them.

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 don't need this constructor anymore anyway. It probably makes sense to replace MemorySpaceData with Kokkos::View at least internally in most places.

@bangerth
Copy link
Member

@masterleinad What do you want to do with this PR?

@masterleinad
Copy link
Member Author

@masterleinad What do you want to do with this PR?

I don't have plans to pursue this further at the moment. The performance results weren't that convincing when I initially opened this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants