Skip to content

Compile All Tests in GitHub CI#1829

Open
fthaler wants to merge 14 commits intoGridTools:masterfrom
fthaler:compile-all-tests
Open

Compile All Tests in GitHub CI#1829
fthaler wants to merge 14 commits intoGridTools:masterfrom
fthaler:compile-all-tests

Conversation

@fthaler
Copy link
Contributor

@fthaler fthaler commented Feb 23, 2026

No description provided.

@gridtoolsjenkins
Copy link
Collaborator

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

@fthaler fthaler changed the title Compile All Test in GitHub CI Compile All Tests in GitHub CI Feb 23, 2026
@fthaler fthaler marked this pull request as ready for review March 2, 2026 13:03
@fthaler fthaler requested review from Copilot and havogt March 2, 2026 13:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the CI build to compile a broader set of test targets across a reduced compiler matrix, while applying compiler-specific workarounds to keep the test suite building on NVHPC/NVCC and newer Clang/GCC.

Changes:

  • Adjust GitHub Actions workflow to build default targets (instead of perftests only) and simplify the compiler/build-type matrix.
  • Add NVHPC-specific CMake filtering of unsupported section/Gc-sections flags for nanobind-static.
  • Update hymap/related tests and refactor icosahedral regression operator functors to improve compilation portability.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit_tests/storage/adapter/CMakeLists.txt NVHPC workaround to strip unsupported compile/link flags from nanobind-static.
tests/unit_tests/common/test_int_vector.cpp Skip a few static_asserts under NVCC to avoid compilation issues.
tests/unit_tests/common/test_hymap.cpp Adjust compilation guards around CTAD/deduction tests.
tests/regression/icosahedral/operators_repository.hpp Refactor from stored std::function members to methods returning lambdas.
tests/regression/icosahedral/lap.cpp Update call sites to use new operators_repository accessor methods.
tests/regression/icosahedral/div.cpp Update call sites to use new operators_repository accessor methods.
tests/regression/icosahedral/curl.cpp Update call sites to use new operators_repository accessor methods.
include/gridtools/stencil/frontend/cartesian/expressions/expr_pow.hpp Fix dependent call to gt_pow<I>::apply.
include/gridtools/common/hymap.hpp Update deduction guide/compiler guards and adjust constraints in values constructors/assignment.
.github/workflows/tests.yml Build default targets in CI and reduce matrix to release-only, fewer compilers.
Comments suppressed due to low confidence (2)

tests/unit_tests/common/test_hymap.cpp:111

  • The deduction test is now enabled for GCC>=12/Clang without excluding NVCC. In CUDA configurations __NVCC__ is defined but __GNUC__ is typically also defined via the host compiler, so this test may become enabled under NVCC and fail to compile (matching the known CTAD limitation noted in hymap.hpp). Consider excluding __NVCC__ here to keep CUDA builds compiling.
#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 12)
        TEST(deduction, smoke) {
            auto testee = hymap::keys<a, b>::values(42, 5.3);

            EXPECT_EQ(42, at_key<a>(testee));
            EXPECT_EQ(5.3, at_key<b>(testee));

            EXPECT_EQ(42, at_key<a>(hymap::keys<a>::values(42)));
        }
#if defined(__clang__) || defined(__GNUC__) && __GNUC__ > 8
        TEST(deduction, empty) { hymap::keys<>::values(); }
#endif
#endif

include/gridtools/common/hymap.hpp:245

  • keys<...>::values::operator= now only constrains Src to be a hymap, but the body assumes every Keys... exists in Src and is assignable to the corresponding Vals. This can turn what used to be a SFINAE’d-away overload into hard compilation errors when assigning from a hymap with missing keys or incompatible value types. Reintroduce constraints that (1) all destination keys are present in Src and (2) each assignment is well-formed/assignable.
            template <class Src>
            constexpr GT_FUNCTION
                std::enable_if_t<!std::is_same_v<values, std::decay_t<Src>> && is_hymap<std::decay_t<Src>>::value,
                    values &>
                operator=(Src &&src) {
                (...,
                    (tuple_util::host_device::get<meta::st_position<meta::list<Keys...>, Keys>::value>(m_vals) =
                            tuple_util::host_device::get<meta::st_position<get_keys<std::decay_t<Src>>, Keys>::value>(
                                src)));

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to +25
foreach(opts COMPILE_OPTIONS INTERFACE_COMPILE_OPTIONS)
get_target_property(nanonbind_opts nanobind-static ${opts})
if (nanonbind_opts)
list(FILTER nanonbind_opts EXCLUDE REGEX "ffunction-sections|fdata-sections")
set_target_properties(nanobind-static PROPERTIES ${opts} "${nanonbind_opts}")
endif()
endforeach()
foreach(opts LINK_OPTIONS INTERFACE_LINK_OPTIONS)
get_target_property(nanobind_opts nanobind-static ${opts})
if (nanobind_opts)
list(FILTER nanobind_opts EXCLUDE REGEX "gc-sections")
set_target_properties(nanobind-static PROPERTIES ${opts} "${nanobind_opts}")
endif()
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

get_target_property() sets the variable to <var>-NOTFOUND when the property is not present. Since if(nanonbind_opts) will still evaluate true for that string, this code can end up filtering/setting the target property to nanonbind_opts-NOTFOUND, unintentionally overwriting the target’s options. Add an explicit -NOTFOUND check (or if (DEFINED ...)) before calling list(FILTER) and set_target_properties().

Copilot uses AI. Check for mistakes.
struct values;

#if !defined(__NVCC__) && defined(__clang__) && __clang_major__ <= 17
#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 12)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

This deduction-guide enablement drops the existing NVCC exclusion, even though the comment right below says NVCC fails CTAD for nested templates. As written, CUDA builds (where __NVCC__ is defined and the host compiler defines __GNUC__) will enable the deduction guide and can reintroduce the NVCC compilation failure. Consider adding !defined(__NVCC__) (or a more specific CUDA compiler guard) to this #if.

Suggested change
#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 12)
#if !defined(__NVCC__) && (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 12))

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +104
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<cells, edges>(i, j, k, c)) {
res += (c == 0 ? 1 : -1) * neighbour.call(u()) * neighbour.call(edge_length());
++e;
}
return res * cell_area_reciprocal()(i, j, k, c);
};
}

auto curl_u() const {
return [this](int i, int j, int k, int c) {
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<vertices, edges>(i, j, k, c)) {
res += (e % 2 ? 1 : -1) * neighbour.call(u()) * neighbour.call(dual_edge_length());
++e;
}
return res * dual_area_reciprocal()(i, j, k, c);
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

These lambdas repeatedly call u(), edge_length(), div_u(), etc. inside loops/expressions, which reconstructs new closure objects each time. Even if optimized, it makes the intent harder to read and can inhibit optimization in some toolchains. Consider creating the needed functors once per returned lambda (e.g., auto u_f = u();) and reusing them within the computation.

Suggested change
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<cells, edges>(i, j, k, c)) {
res += (c == 0 ? 1 : -1) * neighbour.call(u()) * neighbour.call(edge_length());
++e;
}
return res * cell_area_reciprocal()(i, j, k, c);
};
}
auto curl_u() const {
return [this](int i, int j, int k, int c) {
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<vertices, edges>(i, j, k, c)) {
res += (e % 2 ? 1 : -1) * neighbour.call(u()) * neighbour.call(dual_edge_length());
++e;
}
return res * dual_area_reciprocal()(i, j, k, c);
auto u_f = u();
auto edge_length_f = edge_length();
auto cell_area_reciprocal_f = cell_area_reciprocal();
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<cells, edges>(i, j, k, c)) {
res += (c == 0 ? 1 : -1) * neighbour.call(u_f) * neighbour.call(edge_length_f);
++e;
}
return res * cell_area_reciprocal_f(i, j, k, c);
};
}
auto curl_u() const {
return [this](int i, int j, int k, int c) {
auto u_f = u();
auto dual_edge_length_f = dual_edge_length();
auto dual_area_reciprocal_f = dual_area_reciprocal();
double res = 0;
int e = 0;
for (auto &&neighbour : gridtools::neighbours_of<vertices, edges>(i, j, k, c)) {
res += (e % 2 ? 1 : -1) * neighbour.call(u_f) * neighbour.call(dual_edge_length_f);
++e;
}
return res * dual_area_reciprocal_f(i, j, k, c);

Copilot uses AI. Check for mistakes.
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.

3 participants