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

Use different EBO workaround for MSVC (rebased) #3924

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Apr 9, 2021

#3932 rebased on top of #3832.

Fixes #3907.

@masterleinad
Copy link
Contributor Author

Seems to work on Summit for me.

@masterleinad masterleinad removed the [WIP] label Apr 9, 2021
@masterleinad masterleinad changed the title [WIP] Use different EBO workaround for MSVC (rebased) Use different EBO workaround for MSVC (rebased) Apr 9, 2021
@ndellingwood
Copy link
Contributor

Serial xl/16.1.1 build compiled for me without errors. Unrelated to changes in this PR, this warning was output which will error with -Werror:

/ascldap/users/ndellin/kokkos-testing/core/unit_test/tools/printing-tool.cpp:11:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
  auto _pos        = _cmd.find_last_of('/');

@masterleinad
Copy link
Contributor Author

Unrelated to changes in this PR, this warning was output which will error with -Werror: [...]

Can you try adding

diff --git a/core/unit_test/CMakeLists.txt b/core/unit_test/CMakeLists.txt
index b16764f97..5e6e56917 100644
--- a/core/unit_test/CMakeLists.txt
+++ b/core/unit_test/CMakeLists.txt
@@ -704,6 +704,7 @@ KOKKOS_ADD_ADVANCED_TEST( UnitTest_PushFinalizeHook_terminate
       kokkosprinter-tool SHARED
       SOURCES tools/printing-tool.cpp
     )
+    TARGET_COMPILE_FEATURES(kokkosprinter-tool PUBLIC cxx_std_14)
 
     KOKKOS_ADD_TEST_EXECUTABLE(
       ProfilingAllCalls

?

@ndellingwood
Copy link
Contributor

@masterleinad your suggested change resolved the warning, thanks. When I added -Werror it triggered another pre-existing issue

/ascldap/users/ndellin/kokkos-testing/core/unit_test/TestComplex.hpp:417:9: error: unused type alias 'RealType' [-Werror,-Wunused-local-typedef]
  using RealType = double;

resolved by moving the using statement within the if-guard

diff --git a/core/unit_test/TestComplex.hpp b/core/unit_test/TestComplex.hpp
index 8ec2dc7..1218170 100644
--- a/core/unit_test/TestComplex.hpp
+++ b/core/unit_test/TestComplex.hpp
@@ -414,13 +414,13 @@ TEST(TEST_CATEGORY, complex_special_funtions) {
 TEST(TEST_CATEGORY, complex_io) { testComplexIO(); }
 
 TEST(TEST_CATEGORY, complex_trivially_copyable) {
-  using RealType = double;
 
   // Kokkos::complex<RealType> is trivially copyable when RealType is
   // trivially copyable
   // Simply disable the check for IBM's XL compiler since we can't reliably
   // check for a version that defines relevant functions.
 #if !defined(__ibmxl__)
+  using RealType = double;
   // clang claims compatibility with gcc 4.2.1 but all versions tested know
   // about std::is_trivially_copyable.
   ASSERT_TRUE(std::is_trivially_copyable<Kokkos::complex<RealType>>::value ||

@masterleinad
Copy link
Contributor Author

@ndellingwood You should create a pull request since you already have the necessary changes. 🙂

ndellingwood added a commit to ndellingwood/kokkos that referenced this pull request Apr 9, 2021
@ndellingwood
Copy link
Contributor

I put in #3927 with the warning fixes

@masterleinad
Copy link
Contributor Author

Retest this please.

1 similar comment
@masterleinad
Copy link
Contributor Author

Retest this please.

@ndellingwood ndellingwood mentioned this pull request May 24, 2021
3 tasks
@masterleinad masterleinad marked this pull request as ready for review May 25, 2021 15:49
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. This actually seems to solve #3936 but I need to test on more compilers to be sure before closing that issue. Also the code is much more readable now without having to do the linearize bases workaround.

@dalg24 dalg24 merged commit 09766ff into kokkos:develop Jun 11, 2021
@masterleinad masterleinad deleted the msvc_ebo_workaround_rebased branch June 14, 2021 15:43
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

4 participants