Add Matrix-Vector Product example - 1D distribution #158

AdRi1t · 2025-03-26T13:52:00Z

Add AXPY example to demonstrate the use of KokkosComm
Add KokkosComm_ENABLE_EXAMPLES option (default OFF)
~~Modify KokkosComm::wait_all to bypass std::vector~~

- Add AXPY example to demonstrate the use of KokkosComm - Add KokkosComm_ENABLE_EXAMPLES option (default OFF) - Modify KokkosComm::wait_all to bypass std::vector Signed-off-by: Adrien Taberner <[email protected]>

Signed-off-by: Adrien Taberner <[email protected]>

cedricchevalier19

I think that the added wait_all is not required, and even so, it should be in a separate PR.

examples/01_mvp/01_mvp.cpp

cedricchevalier19 · 2025-03-26T16:18:29Z

examples/01_mvp/01_mvp.cpp

+  {
+  using ExecSpace   = Kokkos::DefaultExecutionSpace;
+  using CommSpace   = KokkosComm::DefaultCommunicationSpace;
+  using matrix_type = Kokkos::View<double**, Kokkos::LayoutRight, ExecSpace>;


Is Kokkos::LayoutRight mandatory?

I thought that we can have either storage for the local data even if the global matrix is row distributed.

examples/01_mvp/01_mvp.cpp

cedricchevalier19 · 2025-03-26T16:21:08Z

examples/01_mvp/01_mvp.cpp

+  // Initialize A, x, y
+  Kokkos::parallel_for("Initialize", dim.nb_rows, KOKKOS_LAMBDA(const int i) {
+    for (int j = 0; j < N; j++) {
+      A(i, j) = 1.0;


Perhaps put other value than the same everywhere.

The vector x now corresponds to x = (1,2, ..., N) and the matrix A is filled as A(i,j) = j + 1.0
which means that each element of y is equal to the sum of squares from 1 to N. This is now less trivial, but it can be improved.

cedricchevalier19 · 2025-03-26T16:22:27Z

examples/01_mvp/01_mvp.cpp

+  RankDims current_dim = dim;
+
+  // Communication and computation steps
+  for (int step = 1; step < size; step++) {


What is a step in this algorithm?

Here it's a step of calculations and communications. There's a communication for the next chunk of the distributed x vector. Step 1 performs the calculation on the vector x local to the rank.

In this example, we progress diagonally, starting with the part of x that is assigned to the rank.

cedricchevalier19 · 2025-03-26T16:23:33Z

examples/01_mvp/01_mvp.cpp

+// This example demonstrates how to perform a distributed matrix-vector product (A * x = y)
+// using KokkosComm. The matrix A is distributed among the ranks by blocks of contiguous rows.
+// Each rank owns a part of the vector x and will communicate it with other ranks step by step.
+// At each step a node communicates with two other nodes to send and receive data.


Why not doing collective? And can you precise the data you are talking about?

src/KokkosComm/mpi/req.hpp

examples/CMakeLists.txt

dssgabriel

Just a quick pass, I agree with all of Cedric's remarks.

I have made additional notes regarding the "parsing" of CLI options that I find too convoluted, and the choice of integer types that looks a bit random to me. I would like to have consistent typing, e.g. always use size_t for unsigned stuff and int everywhere else. If you require specific bit widths, make it clear by using the types provided by the cstdint header.

dssgabriel · 2025-03-27T14:00:06Z

examples/01_mvp/01_mvp.cpp

+  long N = -1;
+
+  for (int i = 0; i < argc; i++) {
+    if (strcmp(argv[i], "-N") == 0 && i + 1 < argc) {
+      N = std::atoi(argv[++i]);
+    }
+    if (strcmp(argv[i], "-h") == 0) {
+      std::cout << "KokkosComm dense square matrix-vector product example \n"
+                << "  Usage: " << argv[0] << " [-N <size>] default size is 2^12" << std::endl;
+      return 0;
+    }
+  }
+  checkArgs(N);


This section of code looks messy and unnecessarily complex to me:

Why is N a long instead of an unsigned integer since in checkArgs you check for strict positivity (N > 1)? I would use size_t.

Why use C standard library functions instead of C++ std::string/string_view comparison operator? String views should be the preferred option since they do not need an allocation.

Why is the for loop starting at 0 instead of 1? argv[0] is always the executable name.

Why use a for loop at all since you only check for one of two possible arguments: -N or -h?

examples/01_mvp/01_mvp.cpp

dssgabriel · 2025-03-27T14:08:18Z

examples/01_mvp/01_mvp.cpp

+  using CommSpace   = KokkosComm::DefaultCommunicationSpace;
+  using matrix_type = Kokkos::View<double**, Kokkos::LayoutRight, ExecSpace>;
+  using vector_type = Kokkos::View<double*, Kokkos::LayoutRight, ExecSpace>;
+  using kk_pair     = Kokkos::pair<long, long>;


Why use long instead of a plain int here? If you absolutely need a 64-bit wide type, I would prefer to have int64_t explicitly.

dssgabriel · 2025-03-27T14:09:32Z

examples/01_mvp/01_mvp.cpp

+
+    // Compute with current data while communication may happen in the background
+    Kokkos::parallel_for("MatrixVectorProduct", dim.nb_rows, KOKKOS_LAMBDA(const int i) {
+      for (unsigned int j = 0; j < current_dim.nb_rows; j++) {


Why use an unsigned int in this loop but not in the other ones?
I would suggest size_t instead.

dssgabriel · 2025-03-27T14:09:55Z

examples/01_mvp/01_mvp.cpp

+
+  // Last step
+  Kokkos::parallel_for("MatrixVectorProduct tail", dim.nb_rows, KOKKOS_LAMBDA(const int i) {
+    for (unsigned int j = 0; j < current_dim.nb_rows; j++) {


Same thing here for j.

dssgabriel · 2025-03-27T14:11:35Z

examples/01_mvp/01_mvp.cpp

+  unsigned int nb_rows;
+  unsigned int row_start;
+  unsigned int row_end;


I suggest size_t instead of unsigned int here. If you absolutely need 32-bit wide types, use uint32_t.

Signed-off-by: Adrien Taberner <[email protected]>

dssgabriel · 2025-03-28T14:30:21Z

examples/01_mvp/01_mvp.cpp

      std::cout << "KokkosComm dense square matrix-vector product example \n"
                << "  Usage: " << argv[0] << " [-N <size>] default size is 2^12" << std::endl;
      return 0;
+    } else if (arg == "-N" && argc > 2) {
+      N = static_cast<int>(std::stoi(argv[2]));


I don't think the static_cast is necessary here.

Signed-off-by: Adrien Taberner <[email protected]>

Add AXPY example

cd834f1

- Add AXPY example to demonstrate the use of KokkosComm - Add KokkosComm_ENABLE_EXAMPLES option (default OFF) - Modify KokkosComm::wait_all to bypass std::vector Signed-off-by: Adrien Taberner <[email protected]>

AdRi1t changed the title ~~Dense Distributed Matrix-Vector product - 1D distribution example~~ Add Matrix-Vector Product example - 1D distribution Mar 26, 2025

Fix Typo

597c75f

Signed-off-by: Adrien Taberner <[email protected]>

cedricchevalier19 requested changes Mar 26, 2025

View reviewed changes

dssgabriel requested changes Mar 27, 2025

View reviewed changes

requested change

e89cd29

Signed-off-by: Adrien Taberner <[email protected]>

dssgabriel reviewed Mar 28, 2025

View reviewed changes

Change in typo & no longer just square cases

a2ccb99

Signed-off-by: Adrien Taberner <[email protected]>

AdRi1t marked this pull request as draft April 7, 2025 13:48

AdRi1t requested a review from cedricchevalier19 April 7, 2025 13:48

Add Matrix-Vector Product example - 1D distribution #158

Are you sure you want to change the base?

Add Matrix-Vector Product example - 1D distribution #158

Uh oh!

Conversation

AdRi1t commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedricchevalier19 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dssgabriel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdRi1t commented Mar 26, 2025 •

edited

Loading