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

Convert PreconditionChebyshev::set_initial_guess_kernel to Kokkos #15091

Merged

Conversation

masterleinad
Copy link
Member

No description provided.

Copy link
Member

@peterrum peterrum 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.

Kokkos::RangePolicy<typename MemorySpace::kokkos_space::execution_space,
Kokkos::IndexType<types::global_dof_index>>
policy(0, n_local_elements);
Kokkos::parallel_for(
Copy link
Member

Choose a reason for hiding this comment

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

I hope this won't be the new standard how to write code in deal.II...

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you mind elaborating?

Kokkos::IndexType<types::global_dof_index>>
policy(0, n_local_elements);
Kokkos::parallel_for(
"PreconditionChebyshev::set_initial_guess",
Copy link
Member

Choose a reason for hiding this comment

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

It really feel strange to specify a string for a function call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this helps Kokkos' tools for profiling.

Copy link
Member

Choose a reason for hiding this comment

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

One wished that they had made that the last argument to the function, with a default to "". But having it is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can omit the label anyway (but that's not recommended since it makes identifying kernels in Kokkos tools, Nsight Systems, VTune, etc. much harder).

Copy link
Member

@kronbichler kronbichler 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. I also think that passing strings is much uglier than having the right C++ function name to start with, but lambdas are what they are. On the performance side, these are big enough loops and not in the innermost level of algorithms. Hence I see no problem (my insistence is because the string is more than 22 characters, so it will allocate memory I assume, and thus have a minimal run time of ~3e-7 seconds).

@masterleinad
Copy link
Member Author

Looks good to me. I also think that passing strings is much uglier than having the right C++ function name to start with, but lambdas are what they are.

We could of course replace the lambda with a functor class, but the problem really is that the actual kernel launched is buried in Kokkos' launch mechanism. You would still be able to find it in profiling tools but the effective mangled kernel name will be very long anyway.

@bangerth
Copy link
Member

Is it worth just standardizing on passing __PRETTY_FUNCTION__ everywhere we call these kernels?

@Rombur
Copy link
Member

Rombur commented Apr 14, 2023

Is it worth just standardizing on passing PRETTY_FUNCTION everywhere we call these kernels?

We tried something similar in a different project but the problem is that the names are too long. You can't read them easily in your profiler. Like Daniel said the name is optional but it's very useful.

@kronbichler
Copy link
Member

OK, let's go with this solution, which is a good progress.

@kronbichler kronbichler merged commit 141a921 into dealii:master Apr 15, 2023
14 checks passed
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