[coll-comm] review update:

- fix include guards - update docs - implement copy/move constructors/assignment with tests - add equality test for collective communicators (needed for testing) Co-authored-by: Pratik Nayak <[email protected]>
ginkgo-project · Aug 9, 2024 · 0ad4ee8 · 0ad4ee8
1 parent b61ad41
commit 0ad4ee8
Show file tree

Hide file tree

Showing 10 changed files with 337 additions and 47 deletions.
diff --git a/core/distributed/dense_communicator.cpp b/core/distributed/dense_communicator.cpp
@@ -60,6 +60,27 @@ GKO_INSTANTIATE_FOR_EACH_LOCAL_GLOBAL_INDEX_TYPE(GKO_DECLARE_DENSE_CONSTRUCTOR);
 #undef GKO_DECLARE_DENSE_CONSTRUCTOR
 
 
+DenseCommunicator::DenseCommunicator(DenseCommunicator&& other) noexcept
+    : DenseCommunicator(other.get_base_communicator())
+{
+    *this = std::move(other);
+}
+
+
+DenseCommunicator& DenseCommunicator::operator=(
+    DenseCommunicator&& other) noexcept
+{
+    if (this != &other) {
+        *this = other;
+        std::fill(other.send_sizes_.begin(), other.send_sizes_.end(), 0);
+        std::fill(other.send_offsets_.begin(), other.send_offsets_.end(), 0);
+        std::fill(other.recv_sizes_.begin(), other.recv_sizes_.end(), 0);
+        std::fill(other.recv_offsets_.begin(), other.recv_offsets_.end(), 0);
+    }
+    return *this;
+}
+
+
 DenseCommunicator::DenseCommunicator(
     communicator base, const std::vector<comm_index_type>& recv_sizes,
     const std::vector<comm_index_type>& recv_offsets,
@@ -125,6 +146,22 @@ comm_index_type DenseCommunicator::get_send_size() const
 }
 
 
+bool operator==(const DenseCommunicator& a, const DenseCommunicator& b)
+{
+    return (a.comm_.is_identical(b.comm_) || a.comm_.is_congruent(b.comm_)) &&
+           a.send_sizes_.size() == b.send_sizes_.size() &&
+           a.recv_sizes_.size() == b.recv_sizes_.size() &&
+           std::equal(a.send_sizes_.begin(), a.send_sizes_.end(),
+                      b.send_sizes_.begin()) &&
+           std::equal(a.recv_sizes_.begin(), a.recv_sizes_.end(),
+                      b.recv_sizes_.begin()) &&
+           std::equal(a.send_offsets_.begin(), a.send_offsets_.end(),
+                      b.send_offsets_.begin()) &&
+           std::equal(a.recv_offsets_.begin(), a.recv_offsets_.end(),
+                      b.recv_offsets_.begin());
+}
+
+
 }  // namespace mpi
 }  // namespace experimental
 }  // namespace gko
diff --git a/core/distributed/device_partition.hpp b/core/distributed/device_partition.hpp
@@ -2,8 +2,8 @@
 //
 // SPDX-License-Identifier: BSD-3-Clause
 
-#ifndef GINKGO_PARTITION_HPP
-#define GINKGO_PARTITION_HPP
+#ifndef GKO_CORE_DISTRIBUTED_PARTITION_HPP
+#define GKO_CORE_DISTRIBUTED_PARTITION_HPP
 
 #include <ginkgo/core/distributed/partition.hpp>
 
@@ -89,4 +89,4 @@ to_device_const(
 }  // namespace gko
 
 
-#endif  // GINKGO_PARTITION_HPP
+#endif  // GKO_CORE_DISTRIBUTED_PARTITION_HPP
diff --git a/core/distributed/neighborhood_communicator.cpp b/core/distributed/neighborhood_communicator.cpp
@@ -179,6 +179,54 @@ NeighborhoodCommunicator::create_with_same_type(
 }
 
 
+NeighborhoodCommunicator::NeighborhoodCommunicator(
+    NeighborhoodCommunicator&& other) noexcept
+    : NeighborhoodCommunicator(other.get_base_communicator())
+{
+    *this = std::move(other);
+}
+
+
+NeighborhoodCommunicator& NeighborhoodCommunicator::operator=(
+    NeighborhoodCommunicator&& other) noexcept
+{
+    if (this != &other) {
+        comm_ = std::exchange(other.comm_, MPI_COMM_SELF);
+        // set topology for other comm
+        std::vector<comm_index_type> non_nullptr(1);
+        non_nullptr.resize(0);
+        other.comm_ = create_neighborhood_comm(this->get_base_communicator(),
+                                               non_nullptr, non_nullptr);
+        send_sizes_ = std::exchange(
+            other.send_sizes_, std::vector<distributed::comm_index_type>{});
+        send_offsets_ = std::exchange(
+            other.send_offsets_, std::vector<distributed::comm_index_type>{0});
+        recv_sizes_ = std::exchange(
+            other.recv_sizes_, std::vector<distributed::comm_index_type>{});
+        recv_offsets_ = std::exchange(
+            other.recv_offsets_, std::vector<distributed::comm_index_type>{0});
+    }
+    return *this;
+}
+
+
+bool operator==(const NeighborhoodCommunicator& a,
+                const NeighborhoodCommunicator& b)
+{
+    return (a.comm_.is_identical(b.comm_) || a.comm_.is_congruent(b.comm_)) &&
+           a.send_sizes_.size() == b.send_sizes_.size() &&
+           a.recv_sizes_.size() == b.recv_sizes_.size() &&
+           std::equal(a.send_sizes_.begin(), a.send_sizes_.end(),
+                      b.send_sizes_.begin()) &&
+           std::equal(a.recv_sizes_.begin(), a.recv_sizes_.end(),
+                      b.recv_sizes_.begin()) &&
+           std::equal(a.send_offsets_.begin(), a.send_offsets_.end(),
+                      b.send_offsets_.begin()) &&
+           std::equal(a.recv_offsets_.begin(), a.recv_offsets_.end(),
+                      b.recv_offsets_.begin());
+}
+
+
 template <typename LocalIndexType, typename GlobalIndexType>
 NeighborhoodCommunicator::NeighborhoodCommunicator(
     communicator base,

diff --git a/core/test/mpi/distributed/dense_communicator.cpp b/core/test/mpi/distributed/dense_communicator.cpp
@@ -17,6 +17,22 @@ class DenseCommunicator : public ::testing::Test {
 
     void SetUp() override { ASSERT_EQ(comm.size(), 6); }
 
+    gko::experimental::mpi::DenseCommunicator create_default_comm()
+    {
+        auto part = gko::share(part_type::build_from_global_size_uniform(
+            ref, comm.size(), comm.size() * 3));
+        gko::array<long> recv_connections[] = {{ref, {3, 5, 10, 11}},
+                                               {ref, {0, 1, 7, 12, 13}},
+                                               {ref, {3, 4, 17}},
+                                               {ref, {1, 2, 12, 14}},
+                                               {ref, {4, 5, 9, 10, 16, 15}},
+                                               {ref, {8, 12, 13, 14}}};
+        auto imap = map_type{ref, part, comm.rank(), recv_connections[rank]};
+
+
+        return {comm, imap};
+    }
+
     std::shared_ptr<gko::Executor> ref = gko::ReferenceExecutor::create();
     gko::experimental::mpi::communicator comm = MPI_COMM_WORLD;
     int rank = comm.rank();
@@ -139,6 +155,72 @@ TEST_F(DenseCommunicator, CanConstructFromEmptyEnvelopData)
 }
 
 
+TEST_F(DenseCommunicator, CanTestEquality)
+{
+    auto comm_a = create_default_comm();
+    auto comm_b = create_default_comm();
+
+    ASSERT_EQ(comm_a, comm_b);
+}
+
+
+TEST_F(DenseCommunicator, CanTestInequality)
+{
+    auto comm_a = create_default_comm();
+    auto comm_b = gko::experimental::mpi::DenseCommunicator(comm);
+
+    ASSERT_NE(comm_a, comm_b);
+}
+
+
+TEST_F(DenseCommunicator, CanCopyConstruct)
+{
+    auto spcomm = create_default_comm();
+
+    auto copy(spcomm);
+
+    ASSERT_TRUE(copy == spcomm);
+}
+
+
+TEST_F(DenseCommunicator, CanCopyAssign)
+{
+    auto spcomm = create_default_comm();
+    gko::experimental::mpi::DenseCommunicator copy{comm};
+
+    copy = spcomm;
+
+    ASSERT_TRUE(copy == spcomm);
+}
+
+
+TEST_F(DenseCommunicator, CanMoveConstruct)
+{
+    auto spcomm = create_default_comm();
+    auto moved_from = spcomm;
+    auto empty_comm = gko::experimental::mpi::DenseCommunicator{comm};
+
+    auto moved(std::move(moved_from));
+
+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);
+}
+
+
+TEST_F(DenseCommunicator, CanMoveAssign)
+{
+    auto spcomm = create_default_comm();
+    auto moved_from = spcomm;
+    auto empty_comm = gko::experimental::mpi::DenseCommunicator{comm};
+    gko::experimental::mpi::DenseCommunicator moved{comm};
+
+    moved = std::move(moved_from);
+
+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);
+}
+
+
 TEST_F(DenseCommunicator, CanCommunicateIalltoall)
 {
     auto part = gko::share(part_type::build_from_global_size_uniform(
@@ -179,16 +261,7 @@ TEST_F(DenseCommunicator, CanCommunicateIalltoallWhenEmpty)
 
 TEST_F(DenseCommunicator, CanCreateInverse)
 {
-    auto part = gko::share(part_type::build_from_global_size_uniform(
-        ref, comm.size(), comm.size() * 3));
-    gko::array<long> recv_connections[] = {{ref, {3, 5, 10, 11}},
-                                           {ref, {0, 1, 7, 12, 13}},
-                                           {ref, {3, 4, 17}},
-                                           {ref, {1, 2, 12, 14}},
-                                           {ref, {4, 5, 9, 10, 16, 15}},
-                                           {ref, {8, 12, 13, 14}}};
-    auto imap = map_type{ref, part, comm.rank(), recv_connections[rank]};
-    gko::experimental::mpi::DenseCommunicator spcomm{comm, imap};
+    auto spcomm = create_default_comm();
 
     auto inverse = spcomm.create_inverse();
 

diff --git a/core/test/mpi/distributed/neighborhood_communicator.cpp b/core/test/mpi/distributed/neighborhood_communicator.cpp
@@ -17,6 +17,21 @@ class NeighborhoodCommunicator : public ::testing::Test {
 
     void SetUp() override { ASSERT_EQ(comm.size(), 6); }
 
+    gko::experimental::mpi::NeighborhoodCommunicator create_default_comm()
+    {
+        auto part = gko::share(part_type::build_from_global_size_uniform(
+            ref, comm.size(), comm.size() * 3));
+        gko::array<long> recv_connections[] = {{ref, {3, 5, 10, 11}},
+                                               {ref, {0, 1, 7, 12, 13}},
+                                               {ref, {3, 4, 17}},
+                                               {ref, {1, 2, 12, 14}},
+                                               {ref, {4, 5, 9, 10, 16, 15}},
+                                               {ref, {8, 12, 13, 14}}};
+        auto imap = map_type{ref, part, comm.rank(), recv_connections[rank]};
+
+        return {comm, imap};
+    }
+
     std::shared_ptr<gko::Executor> ref = gko::ReferenceExecutor::create();
     gko::experimental::mpi::communicator comm = MPI_COMM_WORLD;
     int rank = comm.rank();
@@ -120,6 +135,72 @@ TEST_F(NeighborhoodCommunicator, CanConstructFromEmptyEnvelopData)
 }
 
 
+TEST_F(NeighborhoodCommunicator, CanTestEquality)
+{
+    auto comm_a = create_default_comm();
+    auto comm_b = create_default_comm();
+
+    ASSERT_EQ(comm_a, comm_b);
+}
+
+
+TEST_F(NeighborhoodCommunicator, CanTestInequality)
+{
+    auto comm_a = create_default_comm();
+    auto comm_b = gko::experimental::mpi::NeighborhoodCommunicator(comm);
+
+    ASSERT_NE(comm_a, comm_b);
+}
+
+
+TEST_F(NeighborhoodCommunicator, CanCopyConstruct)
+{
+    auto spcomm = create_default_comm();
+
+    auto copy(spcomm);
+
+    ASSERT_TRUE(copy == spcomm);
+}
+
+
+TEST_F(NeighborhoodCommunicator, CanCopyAssign)
+{
+    auto spcomm = create_default_comm();
+    gko::experimental::mpi::NeighborhoodCommunicator copy{comm};
+
+    copy = spcomm;
+
+    ASSERT_TRUE(copy == spcomm);
+}
+
+
+TEST_F(NeighborhoodCommunicator, CanMoveConstruct)
+{
+    auto spcomm = create_default_comm();
+    auto moved_from = spcomm;
+    auto empty_comm = gko::experimental::mpi::NeighborhoodCommunicator{comm};
+
+    auto moved(std::move(moved_from));
+
+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);
+}
+
+
+TEST_F(NeighborhoodCommunicator, CanMoveAssign)
+{
+    auto spcomm = create_default_comm();
+    auto moved_from = spcomm;
+    auto empty_comm = gko::experimental::mpi::NeighborhoodCommunicator{comm};
+    gko::experimental::mpi::NeighborhoodCommunicator moved{comm};
+
+    moved = std::move(moved_from);
+
+    ASSERT_TRUE(moved == spcomm);
+    ASSERT_TRUE(moved_from == empty_comm);
+}
+
+
 TEST_F(NeighborhoodCommunicator, CanCommunicateIalltoall)
 {
     auto part = gko::share(part_type::build_from_global_size_uniform(
@@ -160,16 +241,7 @@ TEST_F(NeighborhoodCommunicator, CanCommunicateIalltoallWhenEmpty)
 
 TEST_F(NeighborhoodCommunicator, CanCreateInverse)
 {
-    auto part = gko::share(part_type::build_from_global_size_uniform(
-        ref, comm.size(), comm.size() * 3));
-    gko::array<long> recv_connections[] = {{ref, {3, 5, 10, 11}},
-                                           {ref, {0, 1, 7, 12, 13}},
-                                           {ref, {3, 4, 17}},
-                                           {ref, {1, 2, 12, 14}},
-                                           {ref, {4, 5, 9, 10, 16, 15}},
-                                           {ref, {8, 12, 13, 14}}};
-    auto imap = map_type{ref, part, comm.rank(), recv_connections[rank]};
-    gko::experimental::mpi::NeighborhoodCommunicator spcomm{comm, imap};
+    auto spcomm = create_default_comm();
 
     auto inverse = spcomm.create_inverse();
 

diff --git a/include/ginkgo/core/base/types.hpp b/include/ginkgo/core/base/types.hpp
@@ -813,7 +813,7 @@ namespace distributed {
 
 
 /**
- * Make mpi::comm_index_type avaiable in this namespace
+ * Make mpi::comm_index_type available in this namespace
  */
 using mpi::comm_index_type;
 

diff --git a/include/ginkgo/core/distributed/collective_communicator.hpp b/include/ginkgo/core/distributed/collective_communicator.hpp
@@ -36,8 +36,9 @@ class CollectiveCommunicator {
     /**
      * Non-blocking all-to-all communication.
      *
-     * The send_buffer must have size get_send_size, and the recv_buffer
-     * must have size get_recv_size.
+     * The send_buffer must have allocated at least get_send_size number of
+     * elements, and the recv_buffer must have allocated at least get_recv_size
+     * number of elements.
      *
      * @tparam SendType  the type of the elements to send
      * @tparam RecvType  the type of the elements to receive
-Original file line number
+Diff line change
@@ Expand Up / @@ -813,7 +813,7 @@ namespace distributed { @@
     /**
-     * Make mpi::comm_index_type avaiable in this namespace
+     * Make mpi::comm_index_type available in this namespace
      */
     using mpi::comm_index_type;
@@ Expand Down @@