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

Cuda: Fix nvcc warnings #7021

Merged
merged 12 commits into from
Jun 12, 2024
Merged

Cuda: Fix nvcc warnings #7021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e899550
Cuda: Fix nvcc warnings
masterleinad May 21, 2024
726a8f2
Fix quotation marks in CXX flags
masterleinad May 22, 2024
1876867
Fix kokkos_swap
masterleinad May 22, 2024
d0d99bd
Fix array size
masterleinad May 22, 2024
13447c4
Fix gtest
masterleinad May 22, 2024
5906cba
Fix .jenkins whitespce
masterleinad May 22, 2024
9d1842e
Only use -Werror all-warnings with explicit nvcc_wrapper
masterleinad May 23, 2024
0e88744
Fix dangling reference
masterleinad May 23, 2024
2a15c75
Suppress 'long double' is treated as 'double' in device code
masterleinad May 23, 2024
5b0d945
Use -Xcudafe --diag_suppress=20208 for 11.6 build; nothing else seems…
masterleinad May 29, 2024
1625ec2
Try moving pragma suppress to tests
masterleinad May 30, 2024
2c3fd02
Use -Xcudafe --diag_suppress=20208 in Makefile build
masterleinad May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .jenkins
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ pipeline {
sh '''rm -rf build && mkdir -p build && cd build && \
../gnu_generate_makefile.bash \
--with-options=compiler_warnings \
--cxxflags="-Werror" \
--cxxflags="-Werror -Werror all-warnings -Xcudafe --diag_suppress=20208" \
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 needed to move pragma push into the Complex.hpp to avoid seeing the warning. At this point, it seems to be better to just disable the warning for this build in the compiler flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does all_warnings accompany the second -Werror? I'm really asking for clarification on why -Werror is needed twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-Werror is for the host compiler and -Werror all-warnings is for the device compiler.

--cxxstandard=c++17 \
--with-cuda \
--with-cuda-options=enable_lambda \
Expand Down Expand Up @@ -448,7 +448,7 @@ pipeline {
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
-DCMAKE_CXX_COMPILER=$WORKSPACE/bin/nvcc_wrapper \
-DCMAKE_CXX_FLAGS=-Werror \
-DCMAKE_CXX_FLAGS="-Werror -Werror all-warnings -Xcudafe --diag_suppress=20208" \
-DCMAKE_CXX_STANDARD=17 \
-DKokkos_ARCH_NATIVE=ON \
-DKokkos_ENABLE_COMPILER_WARNINGS=ON \
Expand Down
2 changes: 1 addition & 1 deletion core/src/Cuda/Kokkos_Cuda.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class Cuda {
///
/// This matches the __CUDA_ARCH__ specification.
KOKKOS_DEPRECATED static size_type device_arch() {
const cudaDeviceProp& cudaProp = Cuda().cuda_device_prop();
const cudaDeviceProp cudaProp = Cuda().cuda_device_prop();
Copy link
Member

Choose a reason for hiding this comment

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

What warning was raised here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dangling reference

return cudaProp.major * 100 + cudaProp.minor;
}

Expand Down
7 changes: 4 additions & 3 deletions core/unit_test/TestArray.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ static_assert(test_array_structured_binding_support());

template <typename L, typename R>
KOKKOS_FUNCTION constexpr bool is_equal(L const& l, R const& r) {
if (std::size(l) != std::size(r)) return false;
if (l.size() != r.size()) return false;

for (size_t i = 0; i != std::size(l); ++i) {
for (size_t i = 0; i != l.size(); ++i) {
if (l[i] != r[i]) return false;
}

Expand Down Expand Up @@ -137,7 +137,8 @@ struct MyInt {
int i;

private:
friend constexpr void kokkos_swap(MyInt& lhs, MyInt& rhs) noexcept {
friend constexpr KOKKOS_FUNCTION void kokkos_swap(MyInt& lhs,
MyInt& rhs) noexcept {
lhs.i = 255;
rhs.i = 127;
}
Expand Down
23 changes: 23 additions & 0 deletions core/unit_test/TestComplex.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@
#include <cstdio>
#include <sstream>

// Suppress "'long double' is treated as 'double' in device code"
#ifdef KOKKOS_COMPILER_NVCC
#ifdef __NVCC_DIAG_PRAGMA_SUPPORT__
#pragma nv_diagnostic push
#pragma nv_diag_suppress 20208
#else
#ifdef __CUDA_ARCH__
#pragma diagnostic push
#pragma diag_suppress 20208
#endif
#endif
#endif

namespace Test {

// Test construction and assignment
Expand Down Expand Up @@ -533,3 +546,13 @@ TEST(TEST_CATEGORY, complex_operations_arithmetic_types_overloads) {
}

} // namespace Test

#ifdef KOKKOS_COMPILER_NVCC
#ifdef __NVCC_DIAG_PRAGMA_SUPPORT__
#pragma nv_diagnostic pop
#else
#ifdef __CUDA_ARCH__
#pragma diagnostic pop
#endif
#endif
#endif
23 changes: 23 additions & 0 deletions core/unit_test/TestNumericTraits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,19 @@
#include <limits>
#include "Kokkos_NumericTraits.hpp"

// Suppress "'long double' is treated as 'double' in device code"
#ifdef KOKKOS_COMPILER_NVCC
#ifdef __NVCC_DIAG_PRAGMA_SUPPORT__
#pragma nv_diagnostic push
#pragma nv_diag_suppress 20208
#else
#ifdef __CUDA_ARCH__
#pragma diagnostic push
#pragma diag_suppress 20208
#endif
#endif
#endif
Comment on lines +24 to +35
Copy link
Member

Choose a reason for hiding this comment

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

I feel that is not the right approach for that test.
We already have all sorts of guards for long double in these, I feel like we should consider reworking them like we did for the math function (I don;t remember the details but I see that it is not part of this PR meaning we somehow resolved it without suppressing the diagnostic altogether)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just disable all tests for long double when we compile with Cuda support (which is what we are doing for other configurations) but these seem to work fine after suppressing the warning.

Copy link
Contributor

@ajpowelsnl ajpowelsnl Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are the long double tests on CUDA already effectively switched off, in that they are run as double in CUDA device code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, everything that uses TestNumericTraits at least, see

kokkos/core/unit_test/TestNumericTraits.hpp

Lines 182 to 204 in 63f0520

#if (defined(KOKKOS_COMPILER_NVCC) && defined(KOKKOS_ENABLE_CUDA)) || \
defined(KOKKOS_ENABLE_SYCL) || defined(KOKKOS_ENABLE_OPENMPTARGET)
template <class Tag>
struct TestNumericTraits<
#if defined(KOKKOS_ENABLE_CUDA)
Kokkos::Cuda,
#elif defined(KOKKOS_ENABLE_SYCL)
Kokkos::Experimental::SYCL,
#else
Kokkos::Experimental::OpenMPTarget,
#endif
long double, Tag> {
template <class T>
using trait = typename Tag::template trait<T>;
TestNumericTraits() {
(void)take_address_of(trait<long double>::value);
// Do nothing on the device.
// According to the doc
// https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#constexpr-variables
// the traits member constant value cannot be directly used in device code.
}
};
#endif
.


struct extrema {
#define DEFINE_EXTREMA(T, m, M) \
KOKKOS_FUNCTION static T min(T) { return m; } \
Expand Down Expand Up @@ -736,3 +749,13 @@ CHECK_NAN_INSTANTIATED_ON_CV_QUALIFIED_TYPES_FLOATING_POINT(signaling_NaN);

#undef CHECK_NAN_INSTANTIATED_ON_CV_QUALIFIED_TYPES_FLOATING_POINT
#undef CHECK_NAN_INSTANTIATED_ON_CV_QUALIFIED_TYPES

#ifdef KOKKOS_COMPILER_NVCC
#ifdef __NVCC_DIAG_PRAGMA_SUPPORT__
#pragma nv_diagnostic pop
#else
#ifdef __CUDA_ARCH__
#pragma diagnostic pop
#endif
#endif
#endif
2 changes: 1 addition & 1 deletion tpls/gtest/gtest/gtest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4910,7 +4910,7 @@ class NeverThrown {
class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) \
: public parent_class { \
public: \
GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default; \
GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() { (void)test_info_; }\
Copy link
Member

Choose a reason for hiding this comment

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

Could you please reference issues or rejected prs on googletest that show that there is no interest on resolving this upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

google/googletest#4104 (comment) mentions that nvcc is an unsupported compiler (in a slightly different context).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be a problem if we later want to update gtest ?

What do you think of pre-installing all Kokkos dependencies in the CI so that we use the flags we want solely on Kokkos ? We could then compile gtest with the host compiler ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be a problem if we later want to update gtest ?

We will notice if this is still a problem or not.

What do you think of pre-installing all Kokkos dependencies in the CI so that we use the flags we want solely on Kokkos ? We could then compile gtest with the host compiler ?

This is not about installing gtest but rather using it. We compiler warnings when we define unit tests.

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 about installing gtest but rather using it. We compiler warnings when we define unit tests.

I had in mind that using an installed gtest would treat gtest include header path as system path thus removing the warnings ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try that (in a separate pull request); it's certainly a bigger change than changing this one line.

~GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() override = default; \
GTEST_DISALLOW_COPY_AND_ASSIGN_(GTEST_TEST_CLASS_NAME_(test_suite_name, \
test_name)); \
Expand Down
Loading