[Refactor] Remove unnecessary manual MPI_Reduce in hsolver dav code#7314
Open
Cstandardlib wants to merge 1 commit intodeepmodeling:developfrom
Open
[Refactor] Remove unnecessary manual MPI_Reduce in hsolver dav code#7314Cstandardlib wants to merge 1 commit intodeepmodeling:developfrom
Cstandardlib wants to merge 1 commit intodeepmodeling:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Davidson-based hsolver MPI reduction logic by removing manual temporary buffers and explicit MPI_Reduce calls, and instead consistently using Parallel_Reduce::reduce_pool for reducing hcc/scc contributions across MPI ranks.
Changes:
- Simplified MPI reduction paths in Davidson solvers by deleting manual
swapbuffer management and explicitMPI_Reducecalls. - Unified reduction calls to
Parallel_Reduce::reduce_pool(...)for both real and complex builds inDiagoDavid::cal_elemandDiago_DavSubspace::cal_elem.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| source/source_hsolver/diago_david.cpp | Replaces manual complex reduction logic with Parallel_Reduce::reduce_pool during hcc accumulation. |
| source/source_hsolver/diago_dav_subspace.cpp | Applies the same reduction simplification for hcc and scc accumulation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3e9f159 to
9db6a0a
Compare
reduce_pool template already has single precision (std::complex<float>) instantiation with MPI_IN_PLACE, so manually creating swap buffers and calling MPI_Reduce for complex types is unnecessary. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <[email protected]>
9db6a0a to
0f2b2ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
diago_dav_subspace.cppanddiago_david.cppreduce_pooltemplate already hasstd::complex<float>andstd::complex<double>instantiations withMPI_IN_PLACE, making manual buffer management redundantRelated Issues
Fixes #7313
Changes
source_hsolver/diago_dav_subspace.cppswapbuffer allocation andsyncmem_complex_opcallstd::is_same<T, double>::valuetype checkMPI_Reducecalls for complex typesParallel_Reduce::reduce_poolfor bothhccandsccsource_hsolver/diago_david.cpphccreduce operationTesting
reduce_poolfunction that was already used fordoubletype