Skip to content

Conversation

@greole
Copy link
Collaborator

@greole greole commented Aug 6, 2025

This PR improves repartitioned multi gpu matrix values and vector value update performance.

@greole greole requested a review from Copilot September 12, 2025 11:29
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

This PR improves performance of repartitioned multi-GPU matrix values and vector value update operations by optimizing communication patterns and adding device communication initialization.

  • Refactors all-to-all communication to use MPI operations (MPI_Igatherv, MPI_Iscatterv) for better performance
  • Adds symmetric matrix optimization to skip redundant data transfers
  • Introduces device communication initialization and repartitioned communicator support

Reviewed Changes

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

Show a summary per file
File Description
test/unit/MatrixWrapper/Distributed.cpp Adds device communication initialization calls to test setup
src/StoppingCriterion.cpp Adds timing measurements for performance analysis
src/MatrixWrapper/Distributed.cpp Implements optimized MPI communication and symmetric matrix handling
src/CommunicationPattern.cpp Adds new function for computing repartitioned all-to-all patterns
include/OGL/lduLduBase.hpp Adds device communication initialization and timing code
include/OGL/StoppingCriterion.hpp Adds frequency mode configuration for stopping criterion optimization
include/OGL/DevicePersistent/Vector.hpp Replaces communication function with direct MPI calls for copy-back operation
include/OGL/DevicePersistent/ExecutorHandler.hpp Adds rank management utilities and device communication setup
include/OGL/CommunicationPattern.hpp Declares new repartitioned communication pattern function
Comments suppressed due to low confidence (1)

src/MatrixWrapper/Distributed.cpp:1

  • Duplicate assignment to recv_offsets.back(). The first assignment on line 203 sets send_offsets.back() but the code incorrectly assigns to recv_offsets.back() twice. Line 203 should assign to send_offsets.back().
// SPDX-FileCopyrightText: 2024 OGL authors

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +345 to +339
std::string("\n\tRetrieve results bandwidth "); // +
std::to_string(bandwidth_copy_back) + std::string(" [GByte/s]");
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

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

The string concatenation is broken. Line 345 ends with a comment // + and line 346 is a separate statement that isn't assigned to anything. The + operator should be on line 345 to concatenate with line 346.

Suggested change
std::string("\n\tRetrieve results bandwidth "); // +
std::to_string(bandwidth_copy_back) + std::string(" [GByte/s]");
std::string("\n\tRetrieve results bandwidth ") +
std::to_string(bandwidth_copy_back) + std::string(" [GByte/s]");

Copilot uses AI. Check for mistakes.
@greole greole changed the title Enh/repart comm Refactor MPI communication Sep 25, 2025
@greole greole changed the base branch from stack/ogl_0.6 to dev September 25, 2025 13:45
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.

2 participants