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

Replace assert with KOKKOS_ASSERT in kernel code #951

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

masterleinad
Copy link
Collaborator

Fixes #949.

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

Might as well also remove the <cassert> header inclusion in files that don't use assert.

@aprokop
Copy link
Contributor

aprokop commented Sep 11, 2023

Would be nice if kokkos/kokkos#5403 was resolved first to not bring Kokkos_Core.hpp.

@masterleinad masterleinad marked this pull request as ready for review September 11, 2023 22:11
@dalg24
Copy link
Contributor

dalg24 commented Sep 11, 2023

Why is it acceptable to pull in the full <Kokkos_Core.hpp> header?
What are the pros and cons as compared to rolling our own?

@masterleinad
Copy link
Collaborator Author

Why is it acceptable to pull in the full <Kokkos_Core.hpp> header? What are the pros and cons as compared to rolling our own?

My two cents:

  • Effectively, any code that uses ArborX needs Kokkos_Core.hpp anyway since that's what you need for a parallel_for or for using View anyway as of now. It's realistic to make the needed Kokkos header public pretty soon and transitioning is very easy since it only means to change an include.
  • It seems that KOKKOS_ASSERT is doing what we want and so coming up with our own solution/macro would probably be pretty much a copy of what Kokkos provides already. If we roll or own, potentially moving to KOKKOS_ASSERT when we only need the Kokkos_Error.hpp (or whatever it will be called) header requires more changes than using it from the beginning.

@dalg24
Copy link
Contributor

dalg24 commented Sep 12, 2023

(Not saying we must define our own but disagreeing with the comment above) Purging the unnecessary header includes is more difficult than search and replace.

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

Should we create ArborX_KokkosExtAssert.hpp to hide the complexity?

#if KOKKOS_VERSION >= 40200
#include <Kokkos_Assert.hpp> // KOKKOS_ASSERT
#else
#include <Kokkos_Core.hpp> // KOKKOS_ASSERT
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider including impl/Kokkos_Error.hpp instead? I suspect that would work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cons:

  • impl/Kokkos_Error.hpp is not a public header. We shouldn't be a bad example.
  • up to Kokkos version 4.2.0 (at the very least), there is not much of a chance for an ArborX executable to be able to include less than Kokkos_Core.hpp anyway.

Pros:

Copy link
Contributor

Choose a reason for hiding this comment

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

Cons:

  • impl/Kokkos_Error.hpp is not a public header. We shouldn't be a bad example.

In our implementation details only. I don't think that is an issue.

  • up to Kokkos version 4.2.0 (at the very least), there is not much of a chance for an ArborX executable to be able to include less than Kokkos_Core.hpp anyway.

I don't understand the point you want to make.

@masterleinad
Copy link
Collaborator Author

Retest this please.

@aprokop aprokop merged commit 4c015c7 into arborx:master Oct 19, 2023
1 check passed
@aprokop aprokop deleted the use_kokkos_assert branch October 19, 2023 17:16
@aprokop
Copy link
Contributor

aprokop commented Oct 19, 2023

HIP did not start, one CUDA build ran out of disk space. Otherwise, good.

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.

Replace use of the macro assert in kernels
3 participants