Skip to content

Commit

Permalink
Cleaning up some warnings and enabling -Werror on CPU CI tests (kokko…
Browse files Browse the repository at this point in the history 
…s#107)

* Fix compilation warnings

* Fix compilation warnings

unused arguments

* Add warnings on CPU CI

* Fix format

* Fix format

* Fix format with correct clang-format version

* Reintroducing some ExecutionSpace

* Better comments

* Remove unneeded comment
  • Loading branch information
@cedricchevalier19
cedricchevalier19 authored Jul 2, 2024
1 parent 16fb3f6 commit 2c616d2
Show file tree
Hide file tree
Showing 12 changed files with 238 additions and 182 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/build_test.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,23 +73,23 @@ jobs:
cxx: g++
cmake_flags:
kokkos: -DKokkos_ENABLE_OPENMP=ON
kokkos_fft: ""
kokkos_fft: -DCMAKE_CXX_FLAGS="-Wall -Wextra" -DCMAKE_COMPILE_WARNING_AS_ERROR=ON
- name: threads
image: gcc
compiler:
c: gcc
cxx: g++
cmake_flags:
kokkos: -DKokkos_ENABLE_THREADS=ON
kokkos_fft: ""
kokkos_fft: -DCMAKE_CXX_FLAGS="-Wall -Wextra" -DCMAKE_COMPILE_WARNING_AS_ERROR=ON
- name: serial
image: gcc
compiler:
c: gcc
cxx: g++
cmake_flags:
kokkos: -DKokkos_ENABLE_SERIAL=ON
kokkos_fft: ""
kokkos_fft: -DCMAKE_CXX_FLAGS="-Wall -Wextra" -DCMAKE_COMPILE_WARNING_AS_ERROR=ON
- name: cuda
image: nvcc
compiler:
Expand Down
15 changes: 8 additions & 7 deletions common/src/KokkosFFT_Helpers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ auto _get_shift(const ViewType& inout, axis_type<DIM> _axes,
assert(!KokkosFFT::Impl::is_out_of_range_value_included(axes, rank));

axis_type<rank> shift = {0};
for (int i = 0; i < DIM; i++) {
for (int i = 0; i < static_cast<int>(DIM); i++) {
int axis = axes.at(i);
shift.at(axis) = inout.extent(axis) / 2 * direction;
}
Expand All @@ -40,7 +40,8 @@ auto _get_shift(const ViewType& inout, axis_type<DIM> _axes,

template <typename ExecutionSpace, typename ViewType>
void _roll(const ExecutionSpace& exec_space, ViewType& inout,
axis_type<1> shift, axis_type<1> axes) {
axis_type<1> shift, axis_type<1>) {
// Last parameter is ignored but present for keeping the interface consistent
static_assert(ViewType::rank() == 1, "_roll: Rank of View must be 1.");
std::size_t n0 = inout.extent(0);

Expand All @@ -56,7 +57,7 @@ void _roll(const ExecutionSpace& exec_space, ViewType& inout,
Kokkos::parallel_for(
Kokkos::RangePolicy<ExecutionSpace, Kokkos::IndexType<std::size_t>>(
exec_space, 0, len),
KOKKOS_LAMBDA(const int& i) {
KOKKOS_LAMBDA(std::size_t i) {
tmp(i + shift0) = inout(i);
if (i + shift1 < n0) {
tmp(i) = inout(i + shift1);
Expand All @@ -70,7 +71,7 @@ void _roll(const ExecutionSpace& exec_space, ViewType& inout,
template <typename ExecutionSpace, typename ViewType, std::size_t DIM1 = 1>
void _roll(const ExecutionSpace& exec_space, ViewType& inout,
axis_type<2> shift, axis_type<DIM1> axes) {
constexpr std::size_t DIM0 = 2;
constexpr int DIM0 = 2;
static_assert(ViewType::rank() == DIM0, "_roll: Rank of View must be 2.");
int n0 = inout.extent(0), n1 = inout.extent(1);

Expand All @@ -90,7 +91,7 @@ void _roll(const ExecutionSpace& exec_space, ViewType& inout,
);

axis_type<2> shift0 = {0}, shift1 = {0}, shift2 = {n0 / 2, n1 / 2};
for (int i = 0; i < DIM1; i++) {
for (int i = 0; static_cast<std::size_t>(i) < DIM1; i++) {
int axis = axes.at(i);

auto [_shift0, _shift1, _shift2] =
Expand Down Expand Up @@ -179,7 +180,7 @@ namespace KokkosFFT {
///
/// \return Sampling frequency
template <typename ExecutionSpace, typename RealType>
auto fftfreq(const ExecutionSpace& exec_space, const std::size_t n,
auto fftfreq(const ExecutionSpace&, const std::size_t n,
const RealType d = 1.0) {
static_assert(std::is_floating_point<RealType>::value,
"fftfreq: d must be float or double");
Expand Down Expand Up @@ -214,7 +215,7 @@ auto fftfreq(const ExecutionSpace& exec_space, const std::size_t n,
///
/// \return Sampling frequency starting from zero
template <typename ExecutionSpace, typename RealType>
auto rfftfreq(const ExecutionSpace& exec_space, const std::size_t n,
auto rfftfreq(const ExecutionSpace&, const std::size_t n,
const RealType d = 1.0) {
static_assert(std::is_floating_point<RealType>::value,
"fftfreq: d must be float or double");
Expand Down
4 changes: 2 additions & 2 deletions common/src/KokkosFFT_normalization.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ void _normalize(const ExecutionSpace& exec_space, ViewType& inout,
}

template <typename ViewType>
auto _coefficients(const ViewType& inout, Direction direction,
Normalization normalization, std::size_t fft_size) {
auto _coefficients(ViewType, Direction direction, Normalization normalization,
std::size_t fft_size) {
using value_type =
KokkosFFT::Impl::real_type_t<typename ViewType::non_const_value_type>;
value_type coef = 1;
Expand Down
2 changes: 1 addition & 1 deletion common/src/KokkosFFT_padding.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ auto get_modified_shape(const InViewType in, const OutViewType /* out */,
}

// Update shapes based on newly given shape
for (int i = 0; i < DIM; i++) {
for (int i = 0; i < static_cast<int>(DIM); i++) {
int positive_axis = positive_axes.at(i);
assert(shape.at(i) > 0);
modified_shape.at(positive_axis) = shape.at(i);
Expand Down
24 changes: 7 additions & 17 deletions common/src/KokkosFFT_transpose.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,14 @@ auto get_map_axes(const ViewType& view, int axis) {
template <class InViewType, class OutViewType, std::size_t DIMS>
void _prep_transpose_view(InViewType& in, OutViewType& out,
axis_type<DIMS> _map) {
constexpr std::size_t rank = OutViewType::rank();
constexpr int rank = OutViewType::rank();

// Assign a View if not a shallow copy
bool is_out_view_ready = true;
std::array<int, rank> out_extents;
for (int i = 0; i < rank; i++) {
out_extents.at(i) = in.extent(_map.at(i));
if (out_extents.at(i) != out.extent(i)) {
if (static_cast<std::size_t>(out_extents.at(i)) != out.extent(i)) {
is_out_view_ready = false;
}
}
Expand All @@ -113,15 +113,15 @@ void _prep_transpose_view(InViewType& in, OutViewType& out,

template <typename ExecutionSpace, typename InViewType, typename OutViewType,
std::enable_if_t<InViewType::rank() == 1, std::nullptr_t> = nullptr>
void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, [[maybe_unused]] axis_type<1> _map) {}
void _transpose(const ExecutionSpace&, InViewType&, OutViewType&,
axis_type<1>) {
// FIXME: Comment why empty?
}

template <typename ExecutionSpace, typename InViewType, typename OutViewType>
void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<2> _map) {
constexpr std::size_t DIM = 2;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;
constexpr std::size_t DIM = 2;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand All @@ -147,7 +147,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<3> _map) {
constexpr std::size_t DIM = 3;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -181,7 +180,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<4> _map) {
constexpr std::size_t DIM = 4;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -217,7 +215,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<5> _map) {
constexpr std::size_t DIM = 5;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -255,7 +252,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<6> _map) {
constexpr std::size_t DIM = 6;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -296,7 +292,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<7> _map) {
constexpr std::size_t DIM = 6;
constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -342,7 +337,6 @@ void _transpose(const ExecutionSpace& exec_space, InViewType& in,
constexpr std::size_t DIM = 6;

constexpr std::size_t rank = InViewType::rank();
using array_layout_type = typename InViewType::array_layout;

using range_type = Kokkos::MDRangePolicy<
ExecutionSpace,
Expand Down Expand Up @@ -409,10 +403,6 @@ template <typename ExecutionSpace, typename InViewType, typename OutViewType,
std::size_t DIM = 1>
void transpose(const ExecutionSpace& exec_space, InViewType& in,
OutViewType& out, axis_type<DIM> _map) {
using in_value_type = typename InViewType::non_const_value_type;
using out_value_type = typename OutViewType::non_const_value_type;
using array_layout_type = typename InViewType::array_layout;

static_assert(Kokkos::is_view<InViewType>::value,
"transpose: InViewType is not a Kokkos::View.");
static_assert(Kokkos::is_view<InViewType>::value,
Expand Down
10 changes: 3 additions & 7 deletions common/src/KokkosFFT_utils.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ struct complex_view_type {
};

template <typename ViewType>
auto convert_negative_axis(const ViewType& view, int _axis = -1) {
auto convert_negative_axis(ViewType, int _axis = -1) {
static_assert(Kokkos::is_view<ViewType>::value,
"convert_negative_axis: ViewType is not a Kokkos::View.");
int rank = static_cast<int>(ViewType::rank());
Expand Down Expand Up @@ -160,7 +160,7 @@ inline std::vector<ElementType> arange(const ElementType start,
ElementType delta = (stop - start) / length;

// thrust::sequence
for (auto i = 0; i < length; i++) {
for (std::size_t i = 0; i < length; i++) {
ElementType value = start + delta * i;
result.push_back(value);
}
Expand All @@ -175,7 +175,6 @@ void conjugate(const ExecutionSpace& exec_space, const InViewType& in,
static_assert(Kokkos::is_view<OutViewType>::value,
"conjugate: OutViewType is not a Kokkos::View.");

using in_value_type = typename InViewType::non_const_value_type;
using out_value_type = typename OutViewType::non_const_value_type;

static_assert(KokkosFFT::Impl::is_complex<out_value_type>::value,
Expand All @@ -189,10 +188,7 @@ void conjugate(const ExecutionSpace& exec_space, const InViewType& in,
Kokkos::parallel_for(
Kokkos::RangePolicy<ExecutionSpace, Kokkos::IndexType<std::size_t>>(
exec_space, 0, size),
KOKKOS_LAMBDA(const int& i) {
out_value_type tmp = in_data[i];
out_data[i] = Kokkos::conj(in_data[i]);
});
KOKKOS_LAMBDA(std::size_t i) { out_data[i] = Kokkos::conj(in_data[i]); });
}

template <typename ViewType>
Expand Down
10 changes: 5 additions & 5 deletions common/unit_test/Test_Helpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ void test_fftshift1D_1DView(int n0) {
_y_ref = {-4, -3, -2, -1, 0, 1, 2, 3, 4};
}

for (std::size_t i = 0; i < n0; i++) {
for (int i = 0; i < n0; i++) {
h_x_ref(i) = static_cast<double>(_x_ref.at(i));
h_y_ref(i) = static_cast<double>(_y_ref.at(i));
}
Expand Down Expand Up @@ -283,8 +283,8 @@ void test_fftshift1D_2DView(int n0) {
5, 6, 7, 8, 9, 10, 11, 12, 13, -13, -12, -11, -10};
}

for (std::size_t i1 = 0; i1 < n1; i1++) {
for (std::size_t i0 = 0; i0 < n0; i0++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
std::size_t i = i0 + i1 * n0;
h_x_ref(i0, i1) = static_cast<double>(_x_ref.at(i));
h_y_axis0_ref(i0, i1) = static_cast<double>(_y0_ref.at(i));
Expand Down Expand Up @@ -340,8 +340,8 @@ void test_fftshift2D_2DView(int n0) {
1, 2, 3, 4, -13, -12, -11, -10, 9, 10, 11, 12, 13};
}

for (std::size_t i1 = 0; i1 < n1; i1++) {
for (std::size_t i0 = 0; i0 < n0; i0++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
std::size_t i = i0 + i1 * n0;
h_x_ref(i0, i1) = static_cast<double>(_x_ref.at(i));
h_y_ref(i0, i1) = static_cast<double>(_y_ref.at(i));
Expand Down
55 changes: 34 additions & 21 deletions common/unit_test/Test_Padding.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1384,8 +1384,9 @@ TEST(CropOrPad3D, 3DView) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) || i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2))
continue;
h_ref_x(i0, i1, i2) = h_x(i0, i1, i2);
}
Expand Down Expand Up @@ -1426,12 +1427,15 @@ TEST(CropOrPad4D, 4DView) {
View4D<double> ref_x("ref_x", n0_new, n1_new, n2_new, n3_new);

auto h_ref_x = Kokkos::create_mirror_view(ref_x);
// TODO: stop loop early
for (int i3 = 0; i3 < n3; i3++) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) || i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2) || i3 >= h_ref_x.extent(3))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2) ||
static_cast<std::size_t>(i3) >= h_ref_x.extent(3))
continue;
h_ref_x(i0, i1, i2, i3) = h_x(i0, i1, i2, i3);
}
Expand Down Expand Up @@ -1480,9 +1484,11 @@ TEST(CropOrPad5D, 5DView) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) || i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2) || i3 >= h_ref_x.extent(3) ||
i4 >= h_ref_x.extent(4))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2) ||
static_cast<std::size_t>(i3) >= h_ref_x.extent(3) ||
static_cast<std::size_t>(i4) >= h_ref_x.extent(4))
continue;
h_ref_x(i0, i1, i2, i3, i4) = h_x(i0, i1, i2, i3, i4);
}
Expand Down Expand Up @@ -1537,9 +1543,12 @@ TEST(CropOrPad6D, 6DView) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) || i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2) || i3 >= h_ref_x.extent(3) ||
i4 >= h_ref_x.extent(4) || i5 >= h_ref_x.extent(5))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2) ||
static_cast<std::size_t>(i3) >= h_ref_x.extent(3) ||
static_cast<std::size_t>(i4) >= h_ref_x.extent(4) ||
static_cast<std::size_t>(i5) >= h_ref_x.extent(5))
continue;
h_ref_x(i0, i1, i2, i3, i4, i5) =
h_x(i0, i1, i2, i3, i4, i5);
Expand Down Expand Up @@ -1599,10 +1608,13 @@ TEST(CropOrPad7D, 7DView) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) || i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2) || i3 >= h_ref_x.extent(3) ||
i4 >= h_ref_x.extent(4) || i5 >= h_ref_x.extent(5) ||
i6 >= h_ref_x.extent(6))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2) ||
static_cast<std::size_t>(i3) >= h_ref_x.extent(3) ||
static_cast<std::size_t>(i4) >= h_ref_x.extent(4) ||
static_cast<std::size_t>(i5) >= h_ref_x.extent(5) ||
static_cast<std::size_t>(i6) >= h_ref_x.extent(6))
continue;
h_ref_x(i0, i1, i2, i3, i4, i5, i6) =
h_x(i0, i1, i2, i3, i4, i5, i6);
Expand Down Expand Up @@ -1666,13 +1678,14 @@ TEST(CropOrPad8D, 8DView) {
for (int i2 = 0; i2 < n2; i2++) {
for (int i1 = 0; i1 < n1; i1++) {
for (int i0 = 0; i0 < n0; i0++) {
if (i0 >= h_ref_x.extent(0) ||
i1 >= h_ref_x.extent(1) ||
i2 >= h_ref_x.extent(2) ||
i3 >= h_ref_x.extent(3) ||
i4 >= h_ref_x.extent(4) ||
i5 >= h_ref_x.extent(5) ||
i6 >= h_ref_x.extent(6) || i7 >= h_ref_x.extent(7))
if (static_cast<std::size_t>(i0) >= h_ref_x.extent(0) ||
static_cast<std::size_t>(i1) >= h_ref_x.extent(1) ||
static_cast<std::size_t>(i2) >= h_ref_x.extent(2) ||
static_cast<std::size_t>(i3) >= h_ref_x.extent(3) ||
static_cast<std::size_t>(i4) >= h_ref_x.extent(4) ||
static_cast<std::size_t>(i5) >= h_ref_x.extent(5) ||
static_cast<std::size_t>(i6) >= h_ref_x.extent(6) ||
static_cast<std::size_t>(i7) >= h_ref_x.extent(7))
continue;
h_ref_x(i0, i1, i2, i3, i4, i5, i6, i7) =
h_x(i0, i1, i2, i3, i4, i5, i6, i7);
Expand Down
Loading

0 comments on commit 2c616d2

Please sign in to comment.