kokkos / kokkos-comm Goto Github PK

See proposal in kokkos/kokkos#6933 and clangd documentation https://clangd.llvm.org/guides/include-cleaner#iwyu-pragmas.

This project may be the right time to depart from KokkosCore's clang-format-8 requirement and upgrade it to something newer.

I don't know if there are particular (legal?) constraints for such a change, but I suggest we move to at least clang-format-11 (LLVM began supporting -std=c++20 with clang-11). We could go even newer, e.g. with clang-format-14.

Another (personal) suggestion: we should increase the column limit from 80 to 100 (some might even want 120). With current monitor sizes/resolutions, there's no real reason to keep such a narrow width. Moreover, modern C++ can be quite verbose and the 80-column limit causes lots of unnecessary line breaks.

Updating our clang-format while we still have a relatively small codebase is judicious.

PR #30 skips tests for ready-mode send operations as we lack irecv to correctly test the ready-mode send and isend functions.

Unit tests should look something like the following:

if (rank == 0) {
  // Ensure `irecv` has started when we exit the following barrier
  KokkosComm::Impl::barrier(Kokkos::DefaultExecutionSpace(), MPI_COMM_WORLD);

  // Send the view
  KokkosComm::send<KokkosComm::CommMode::Ready>(
    Kokkos::DefaultExecutionSpace(),
    view,
    dst,
    tag,
    MPI_COMM_WORLD
  );
} else {
  // Start reception
  auto req = KokkosComm::irecv(
    Kokkos::DefaultExecutionSpace(),
    view,
    src,
    tag,
    MPI_COMM_WORLD
  );

  // Barrier guaranteeing to the sender (rank 0) that the recipient (rank 1) is ready
  KokkosComm::Impl::barrier(Kokkos::DefaultExecutionSpace(), MPI_COMM_WORLD);

  // Wait until receive completion
  req.wait();
}

These tests should not be skipped anymore once #32 is merged.

In no particular order:

If Kokkos has been installed with mdspan support, i.e. Kokkos_ENABLE_IMPL_MDSPAN=ON, none of the configuration works.

Adding a new mdspan results in duplicated symbols given mdspan headers are protected with #pragma once and the header can be found twice.

Disabling the FetchContent for mdspan does not work either.

Hi there!
After a quick look at the code, it seems like the current implementation requires a C++20 compiler. I was wondering if this is a deliberate choice (to deviate from the general KokkosCore policy I mean) or just the result of the current experimental status of the project. As some GPU compilers do not support C++20, it would be nice to require only a C++17 compiler for now IMHO.
Working on a daily basis with MPI and Kokkos for a few years now, I have developed similar wrappers for our new CFD solver and I'd be very happy to discuss and help develop this project!
Best regards,
Alex

@aprokop

• MPI_Alltoall
• MPI_Barrier
• MPI_Allgather (#39)
• MPI_Irecv
• MPI_Isend

probably skip these for a bare MPI wrapper

• MPI_Comm_dup / MPI_Comm_free
• MPI_Init
• MPI_Finalize

In cmake/KokkosComm_version.hpp.in I think

• do CMake-fu to get the install target to do something
• write a CI test to make sure the install target doesn't break

The motivation here is to prove we have a reasonable implementation by being able to run Trilinos on top of KokkosComm

Hi! I notice that the CommMode enum does not include the buffered mode. Is this intentional or just an oversight?
Best regards,
Alex

Some thoughts I had while browsing through the code:

• How do these operations interact with the execution space? For example, is the user required to fence before calling any consuming operation (like isend)? Non-packing isend is not interacting with the execution space (https://github.com/cwpearson/kokkos-comm/blob/develop/src/impl/KokkosComm_isend.hpp#L54)
• Do you define that the semantics are the exact same as prescribed by the MPI standard or do you allow deviation? One such deviation to consider could be the use of mpi_assert_allow_overtaking option in MPI 4.0 (disabling ordering of same-tag messages). Either way, the semantics should be well-defined.
• Do you require the MPI implementation to be device-aware? This should be documented since there are implementations (or configurations of implementations) that may not support the underlying device hardware.