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

#12: remove BVH_ENABLE_KOKKOS macro #13

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Conversation

cz4rs
Copy link
Collaborator

@cz4rs cz4rs commented Dec 12, 2023

fixes #12

@cz4rs cz4rs force-pushed the 12-remove-bvh-enable-kokkos-macro branch from 481a468 to 457afc9 Compare December 13, 2023 15:56
@cz4rs cz4rs changed the base branch from main to clustering December 13, 2023 16:23
@cz4rs cz4rs requested a review from nmm0 December 13, 2023 16:24
@nmm0
Copy link
Collaborator

nmm0 commented Dec 13, 2023

The failing PR is because of the bottleneck in NimbleSM right now (NimbleSM/NimbleSM#355)

Copy link
Collaborator

@nmm0 nmm0 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!

#include <Kokkos_Core.hpp>
#endif

namespace bvh
{
#if 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, we probably should have removed this dead code a long time ago

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the #if 0 block completely.

@cz4rs cz4rs force-pushed the 12-remove-bvh-enable-kokkos-macro branch 2 times, most recently from 4bcb99d to 6e323e8 Compare December 14, 2023 10:30
@cz4rs cz4rs changed the title Remove BVH_ENABLE_KOKKOS macro #12: remove BVH_ENABLE_KOKKOS macro Dec 27, 2023
@cz4rs cz4rs changed the base branch from clustering to main January 22, 2024 12:40
@cz4rs cz4rs force-pushed the 12-remove-bvh-enable-kokkos-macro branch from 6e323e8 to 70a9b4f Compare January 22, 2024 12:40
@nmm0
Copy link
Collaborator

nmm0 commented Jan 25, 2024

Can you rebase? I (finally)) merged the clustering branch

@cz4rs cz4rs force-pushed the 12-remove-bvh-enable-kokkos-macro branch from 70a9b4f to 9c337dd Compare January 25, 2024 18:03
Copy link
Collaborator

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Looks great! Just the one #if defined that should be changed

src/bvh/hash.hpp Outdated

#if !defined(BVH_ENABLE_KOKKOS) || defined(KOKKOS_ACTIVE_EXECUTION_MEMORY_SPACE_HOST)
#if defined(KOKKOS_ACTIVE_EXECUTION_MEMORY_SPACE_HOST)
Copy link
Collaborator

Choose a reason for hiding this comment

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

KOKKOS_ACTIVE_EXECUTION_MEMORY_SPACE_HOST was removed a while ago and I guess I forgot to remove it from here. I think the best bet is to change this to defined(__GNUC__) since I believe that will work for both gcc and clang host compilers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced with __GNUC__ 👌

@cz4rs cz4rs requested a review from nmm0 January 25, 2024 19:17
Copy link
Collaborator

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nmm0 nmm0 merged commit f10724e into main Jan 25, 2024
3 checks passed
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.

remove BVH_ENABLE_KOKKOS macro
2 participants