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

core/src/impl: Conditionally define get_gpu in Kokkos_Core #3072

Merged
merged 2 commits into from
May 29, 2020

Conversation

e10harvey
Copy link
Contributor

@e10harvey e10harvey commented May 29, 2020

This PR conditionally defines get_gpu since a subset of the kokkos-kernels nightly tests do not enable CUDA, ROCM, or HIP and fail to build due to get_gpu being defined but not used.

This was tested by running spot-checks from kokkos-kernels:

KokkosKernels Repository Status:  190629e7ce8bd49a8cf5f42cccae260672171251 Merge pull request #728 from e10harvey/issue-727

Kokkos Repository Status:  cb9727fae308ce7ae2248dbb8168c430d958bc32 core/src/impl: Conditionally define get_gpu in Kokkos_Core


Going to test compilers:  gcc/7.2.0
Testing compiler gcc/7.2.0
  Starting job gcc-7.2.0-OpenMP-release
kokkos devices: OpenMP
kokkos arch: Power8
kokkos options: 
kokkos cuda options: 
kokkos cxxflags: -O3 -Wall -Wshadow -pedantic -Werror -Wsign-compare -Wtype-limits -Wignored-qualifiers -Wempty-body -Wclobbered -Wuninitialized 
extra_args: 
kokkoskernels scalars: 'complex_double'
kokkoskernels ordinals: int
kokkoskernels offsets: int,size_t
kokkoskernels layouts: LayoutLeft
  PASSED gcc-7.2.0-OpenMP-release
#######################################################
PASSED TESTS
#######################################################
gcc-7.2.0-OpenMP-release build_time=153 run_time=75
KokkosKernels Repository Status:  11c83504a722cd84e68bdbacdfc61ad30b4d79f8 Merge pull request #723 from jjwilke/install-testing

Kokkos Repository Status:  cb9727fae308ce7ae2248dbb8168c430d958bc32 core/src/impl: Conditionally define get_gpu in Kokkos_Core


Going to test compilers:  gcc/7.3.0
Testing compiler gcc/7.3.0
  Starting job gcc-7.3.0-OpenMP-release
kokkos devices: OpenMP
kokkos arch: SNB,Volta70
kokkos options: 
kokkos cuda options: 
kokkos cxxflags: -O3 -Wall -Wshadow -pedantic -Werror -Wsign-compare -Wtype-limits -Wignored-qualifiers -Wempty-body -Wclobbered -Wuninitialized 
extra_args: 
kokkoskernels scalars: 'double,complex_double'
kokkoskernels ordinals: int
kokkoskernels offsets: int,size_t
kokkoskernels layouts: LayoutLeft
  PASSED gcc-7.3.0-OpenMP-release
  Starting job gcc-7.3.0-Pthread-release
kokkos devices: Pthread
kokkos arch: SNB,Volta70
kokkos options: 
kokkos cuda options: 
kokkos cxxflags: -O3 -Wall -Wshadow -pedantic -Werror -Wsign-compare -Wtype-limits -Wignored-qualifiers -Wempty-body -Wclobbered -Wuninitialized 
extra_args: 
kokkoskernels scalars: 'double,complex_double'
kokkoskernels ordinals: int
kokkoskernels offsets: int,size_t
kokkoskernels layouts: LayoutLeft
  PASSED gcc-7.3.0-Pthread-release
#######################################################
PASSED TESTS
#######################################################
gcc-7.3.0-OpenMP-release build_time=138 run_time=60
gcc-7.3.0-Pthread-release build_time=123 run_time=53

Unable to select reviewers: @crtrott, @bartlettroscoe.

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

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

Why not just ifdef the entire function for GPU builds?

@e10harvey e10harvey force-pushed the topic/swat_unused_get_gpu_bug branch from a5fdee1 to 32468fa Compare May 29, 2020 13:55
@@ -168,6 +168,7 @@ int get_ctest_gpu(const char* local_rank_str) {

namespace {

__attribute__ ((unused))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not portable and doesn't work for MSVC. Just guard it with

#if defined(KOKKOS_ENABLE_CUDA) || defined(KOKKOS_ENABLE_HIP)

@e10harvey e10harvey force-pushed the topic/swat_unused_get_gpu_bug branch from 32468fa to cb9727f Compare May 29, 2020 14:01
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

The indentation needs to be fixed:

diff --git a/core/src/impl/Kokkos_Core.cpp b/core/src/impl/Kokkos_Core.cpp
index b4d93f35..2f9d4e1c 100644
--- a/core/src/impl/Kokkos_Core.cpp
+++ b/core/src/impl/Kokkos_Core.cpp
@@ -168,8 +168,7 @@ int get_ctest_gpu(const char* local_rank_str) {
 
 namespace {
 
-#if defined(KOKKOS_ENABLE_CUDA) || \
-    defined(KOKKOS_ENABLE_ROCM) || \
+#if defined(KOKKOS_ENABLE_CUDA) || defined(KOKKOS_ENABLE_ROCM) || \
     defined(KOKKOS_ENABLE_HIP)
 int get_gpu(const InitArguments& args) {
   int use_gpu           = args.device_id;
@@ -210,7 +209,7 @@ int get_gpu(const InitArguments& args) {
   }
   return use_gpu;
 }
-#endif // KOKKOS_ENABLE_CUDA || KOKKOS_ENABLE_ROCM || KOKKOS_ENABLE_HIP
+#endif  // KOKKOS_ENABLE_CUDA || KOKKOS_ENABLE_ROCM || KOKKOS_ENABLE_HIP
 
 bool is_unsigned_int(const char* str) {
   const size_t len = strlen(str);

Please apply the above patch or run clang-format 8.

@@ -168,6 +168,9 @@ int get_ctest_gpu(const char* local_rank_str) {

namespace {

#if defined(KOKKOS_ENABLE_CUDA) || \
defined(KOKKOS_ENABLE_ROCM) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this has no effect and the ROCM related implementations will be removed (in favor of HIP) in the future anyway. Since there are still a lot of places that have it, it doesn't matter that much, though.

@e10harvey e10harvey changed the title src/impl: Mark get_gpu as unused in Kokkos_Core core/src/impl: Conditionally define get_gpu in Kokkos_Core May 29, 2020
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Thanks!

@dalg24
Copy link
Member

dalg24 commented May 29, 2020

OK to test

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #3072 into develop will increase coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #3072     +/-   ##
=========================================
+ Coverage     82.4%   82.7%   +0.2%     
=========================================
  Files          122     122             
  Lines         8095    8074     -21     
=========================================
+ Hits          6672    6678      +6     
+ Misses        1423    1396     -27     
Flag Coverage Δ
#clang 81.5% <ø> (+<0.1%) ⬆️
#gcc 82.9% <ø> (+0.2%) ⬆️
Impacted Files Coverage Δ
core/src/impl/Kokkos_Core.cpp 37.9% <ø> (+1.5%) ⬆️
core/src/Kokkos_MemoryPool.hpp 89.8% <0.0%> (+0.4%) ⬆️
core/src/impl/Kokkos_MultipleTaskQueue.hpp 97.5% <0.0%> (+4.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24ad00c...b9299f5. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants