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

Drop Instruction Set Architecture (ISA) macros #4981

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Apr 27, 2022

Proposing an alternate resolution to #4947

Macros KOKKOS_{USE -> ENABLE}_ISA_{X86_64,KNC,POWERPCLE,POWERPCBE} definitions (#define KOKKOS_USE_ISA_*) do not match guarding in code (#ifdef KOKKOS_ENABLE_ISA_*)
It appears to have been broken for (ever?) a long time.

#4947 proposes to define KOKKOS_ENABLE_ISA_* next to the existing KOKKOS_USE_ISA_* macros. (PR was only dealing with the X86_64 macro which being actually used in our codebase.

This PR suggests to remove these macros altogether rather than trying to "fix" the situation. Pre-desul atomics source files were intentionally left out.

@dalg24
Copy link
Member Author

dalg24 commented Apr 27, 2022

(Applied clang-format)

Macros KOKKOS_{USE -> ENABLE}_ISA_{X86_64,KNC,POWERPCLE,POWERPCBE} definitions (KOKKOS_USE_ISA_*) did not match guarding in code (KOKKOS_ENABLE_ISA_*)
Proposing to remove altogether rather than trying to "fix".  Pre-desul atomics source files were intentionally left out.
@dalg24
Copy link
Member Author

dalg24 commented Apr 27, 2022

Failure (fetnat03 out of space) is clearly unrelated

Copy link
Contributor

@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 -- out of curiosity why were atomic headers and Kokkos_Memory_Fence.hpp excluded? The macro is never enabled so I thought it would be safe to remove from these files.

@dalg24
Copy link
Member Author

dalg24 commented Apr 27, 2022

Because I expect we will remove these files in the near future. Desul atomics are used unless the user explicitly
opt out (configure with -DKokkos_ENABLE_IMPL_DESUL_ATOMICS=OFF). Our intent was to have the option as a contingency plan when we moved to Desul in release 3.5

@dalg24 dalg24 merged commit bb2324e into kokkos:develop Apr 27, 2022
@dalg24 dalg24 deleted the drop_isa_macros branch April 27, 2022 22:52
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.

None yet

3 participants