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::abort directly #14772

Merged
merged 1 commit into from Feb 25, 2023

Conversation

masterleinad
Copy link
Member

Part of #14751. This reverts some changes in #14753.
CUDA and HIP device functions can only be used in a different translation unit if special flags are passed that allow this. However, allowing relocatable device code oftentimes prevents optimization. In particular, it doesn't seem worth it if we would only need it for kokkos_abort. Thus, this pull request uses Kokkos::abort directly at the cost of including Kokkos_Core.hpp. An alternative would be to just ignore Assert in kernels executed on the device.

@masterleinad
Copy link
Member Author

/rebuild

2 similar comments
@masterleinad
Copy link
Member Author

/rebuild

@masterleinad
Copy link
Member Author

/rebuild

@tjhei
Copy link
Member

tjhei commented Feb 16, 2023

/rebuild

It looks like the MPI build might be hanging. Can this be related to this PR?

@masterleinad
Copy link
Member Author

It looks like the MPI build might be hanging. Can this be related to this PR?

This pull request should only inline the call to Kokkos::abort so it shouldn't change any existing behavior.

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

This is of course a bit disappointing, because Kokkos_Core.hpp pulls in many thousands of lines of code. Is the expected increase in compile time measurable?

include/deal.II/base/exceptions.h Outdated Show resolved Hide resolved
@masterleinad
Copy link
Member Author

This is of course a bit disappointing, because Kokkos_Core.hpp pulls in many thousands of lines of code. Is the expected increase in compile time measurable?

We are trying to address that in the next Kokkos release by providing a separate header file.

Locally I am seeing,

real	9m23.509s
user	40m42.230s
sys	2m1.631s

for a debug build of the library and

real	9m7.645s
user	39m56.241s
sys	1m55.335s

without this patch.

@bangerth
Copy link
Member

That seems acceptable to me. I'm ok with merging, but maybe we can get a second vote.

@bangerth
Copy link
Member

Thank you also for testing this out!

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.

I think the increase in compile time is not nice, but acceptable, so let us move forward here.

@kronbichler kronbichler merged commit 68d4128 into dealii:master Feb 25, 2023
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

4 participants