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.

Sign up for GitHub

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

Jump to bottom

Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one KOKKOS_COMPILER* macro #5906

Merged
merged 2 commits into from
Apr 26, 2023
Merged

Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one KOKKOS_COMPILER* macro #5906

merged 2 commits into from
Apr 26, 2023

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Feb 22, 2023 •
edited

As discussed this pull request defines a new compiler macro that allows distinguishing between the Intel Classic Compiler (icpc) and the Intel oneAPI Compiler (icpx). I went through the places that use KOKKOS_COMPILER_INTEL_LLVM and replaced/added/omitted where I saw fit.

Also, this pull request makes sure that only at most one compiler macro KOKKOS_COMPILER* is defined (apart from KOKKOS_COMPILER_NVCC which can appear additionally). I'm happy to separate this change from this pull request if preferred.

@masterleinad masterleinad marked this pull request as draft February 22, 2023 20:31
@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from ada9a07 to fae8ed1 Compare February 22, 2023 20:49
PhilMiller
PhilMiller reviewed Feb 22, 2023
View reviewed changes
core/src/Kokkos_Macros.hpp

#if defined(__APPLE_CC__)
#elif defined(__APPLE_CC__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're now making these definitions strictly mutually exclusive, please check that any places that were testing KOKKOS_COMPILER_CLANG are suitably adapted to check defined(KOKKOS_COMPILER_CLANG) || defined(KOKKOS_COMPILER_APPLECC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did now. We only really check for Clang when CUDA or OpenMPTarget is enabled and AppleClang wouldn't work in these cases anyway. There is of course also the check for builtins that need some love anyway.

PhilMiller
PhilMiller reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Things generally look good now. I haven't grepped for all of the uses of the affected macros myself to confirm that there are no missing updates.

example/build_cmake_in_tree/cmake_example.cpp Outdated Show resolved Hide resolved
@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from 11786bb to 05b6466 Compare February 23, 2023 03:27
@PhilMiller PhilMiller mentioned this pull request Feb 28, 2023
Merged
@PhilMiller
Copy link
Contributor

I've checked over all of the uses of KOKKOS_COMPILER_INTEL in Kokkos and Kokkos Kernels. Everything else in Kokkos is good, with maybe some #if defined(KOKKOS_COMPILER_INTEL gaining || defined(KOKKOS_COMPILER_INTEL_LLVM), but none of them needing to be substituted or whatever. The two uses in Kernels will end up being simplified, because they're currently defined(INTEL) && !defined(clang).

There's one non-Kokkos usage of the macro in Trilinos to work around an Intel ICE in the Kokkos 3.5 PR
https://github.com/trilinos/Trilinos/blame/master/packages/intrepid2/src/Shared/Intrepid2_Data.hpp#L1688
I'll just post a PR to try removing that, and see if it blows up in testing.

@PhilMiller PhilMiller mentioned this pull request Feb 28, 2023
Merged
@masterleinad masterleinad marked this pull request as ready for review February 28, 2023 07:32
@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from 05b6466 to 98a6c4f Compare February 28, 2023 07:34
dalg24
dalg24 requested changes Feb 28, 2023
View reviewed changes
core/src/Kokkos_BitManipulation.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_Macros.hpp Show resolved Hide resolved
@masterleinad masterleinad changed the title Define KOKKOS_COMPILER_INTEL_LLVM Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one *COMPILER* macro Feb 28, 2023
@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from 98a6c4f to a4749f4 Compare February 28, 2023 23:25
@masterleinad masterleinad changed the title Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one *COMPILER* macro Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one KOKKOS_COMPILER* macro Feb 28, 2023
@PhilMiller
Copy link
Contributor

Would be good to fix the commit message to reflect the full scope of the PR.

@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from a4749f4 to e950e24 Compare March 1, 2023 21:16
dalg24
dalg24 previously requested changes Apr 6, 2023
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Please add a unit_tests/TestCompilerMacros.cpp and check that one and only one macro is being defined.

core/src/Kokkos_BitManipulation.hpp Outdated Show resolved Hide resolved
@masterleinad masterleinad requested a review from dalg24 April 6, 2023 12:28
@masterleinad masterleinad mentioned this pull request Apr 6, 2023
Merged
crtrott
crtrott requested changes Apr 6, 2023
View reviewed changes
Copy link
Member

@crtrott crtrott 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 those other macros like ivdep should be defined for Intel LLVM

core/src/Kokkos_Macros.hpp Outdated
@@ -176,19 +172,17 @@
//----------------------------------------------------------------------------
// Intel compiler macros

// FIXME_INTEL_LLVM: Figure out whether these pragmas should get set similarly
// to the classic Intel compiler
Copy link
Member

Choose a reason for hiding this comment

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

yes

@masterleinad masterleinad force-pushed the define_kokkos_compiler_intel_llvm branch from dbd562b to d80f580 Compare April 8, 2023 18:25
@masterleinad
Copy link
Contributor Author

Only gcc-8.4.0 is failing with

KokkosContainers_UnitTest_OpenMP (Child killed)

@masterleinad masterleinad added this to the Release 4.1 milestone Apr 14, 2023
rgayatri23
rgayatri23 reviewed Apr 20, 2023
View reviewed changes
core/src/Kokkos_Macros.hpp
// Very old define
#define KOKKOS_COMPILER_INTEL __ECC
#endif
#define KOKKOS_COMPILER_INTEL_LLVM __INTEL_LLVM_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I will follow this concept for the Cray one too in #5889

@dalg24 dalg24 dismissed their stale review April 26, 2023 19:22

Changes requested were implemented

@dalg24
Copy link
Member

dalg24 commented Apr 26, 2023

Ignoring GCC-8.4.0 failure which is clearly unrelated (running out of memory)
This was resolved elsewhere

@dalg24 dalg24 merged commit 220495f into kokkos:develop Apr 26, 2023
25 checks passed
@masterleinad masterleinad mentioned this pull request May 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crtrott crtrott crtrott approved these changes

@rgayatri23 rgayatri23 rgayatri23 approved these changes

@PhilMiller PhilMiller Awaiting requested review from PhilMiller

@dalg24 dalg24 Awaiting requested review from dalg24

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 4.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@masterleinad @PhilMiller @dalg24 @crtrott @rgayatri23