Skip to content

Refactor parallel communicators - #7893

Open
mohanchen wants to merge 11 commits into
deepmodeling:developfrom
mohanchen:2026-09-01-line1
Open

Refactor parallel communicators#7893
mohanchen wants to merge 11 commits into
deepmodeling:developfrom
mohanchen:2026-09-01-line1

Conversation

@mohanchen

Copy link
Copy Markdown
Collaborator

Refactor parallel communicators

abacus_fixer added 3 commits September 1, 2026 21:25
Scope: 18 files changed, +75 -134 (net -59 lines). No behavior change.
Build: verified with cmake --build build (exit=0), abacus_basic_para built.

Parallel common (模板化去重)
- parallel_common.cpp: collapse the 6 per-type bcast_* copy-paste
  wrappers (int / double / complex<double>, scalar + array) into one
  template <typename T> bcast_world_impl backed by Parallel_Reduce's
  existing MPI_Type<T> traits. Keep bcast_bool/string/char as bespoke
  helpers and drop the redundant extra assignment in bcast_bool.
- test_parallel/CMakeLists.txt (MODULE_BASE_ParaCommon): list the
  transitive objects the standalone test now needs - parallel_reduce,
  parallel_comm, parallel_global, tool_quit, global_file,
  global_function, memory_recorder, timer - because parallel_common.cpp
  references Parallel_Reduce::MPI_Type<T>::value, which depends on the
  six global MPI_Comm in parallel_comm.cpp, which calls
  Parallel_Global::divide_mpi_groups.

Parallel 2D (收紧头文件依赖, rule 3)
- parallel_2d.h: drop the unused #include "source_base/parallel_comm.h"
  (parallel_2d.h only needed <mpi.h> for MPI_Comm); this was the single
  largest conduit that pulled POOL_WORLD/KP_WORLD/INT_BGROUP/BP_WORLD/
  GRID_WORLD/DIAG_WORLD declarations into every user of Parallel_2D /
  Parallel_Orbitals.
- Explicitly include source_base/parallel_comm.h in the 13 consumers
  that were relying on transitive include to reference the global
  communicators: write_hs.h, hsolver_lrtd.hpp, sto_iter.cpp /
  sto_tool.cpp / sto_dos.cpp / sto_elecond.cpp, chgmixing.cpp,
  hsolver_pw_sdft.cpp, esolver_sdft_pw.cpp, diago_bpcg_test.cpp,
  test_hsolver_sdft.cpp.

Parallel grid (重复逻辑收敛 + 现代C++清理)
- parallel_grid.h/cpp: merge zpiece_to_all and zpiece_to_stogroup into
  one zpiece_distribute(zpiece, iz, rho, is_sdft). The only
  difference between the two (~130 lines each) is the choice of
  communicator (MPI_COMM_WORLD vs INT_BGROUP) and the root rank used
  in the non-pool-0 receive path (MY_RANK vs RANK_IN_BPGROUP); both
  are selected with two local variables so the four send/recv
  branches (pool0 root copy, other-rank recv, pool-root multicast,
  other-pool recv) share one implementation. Also rename duplicate
  "case 2" labels into "case 2 / case 3".
- parallel_grid.cpp::z_distribution: replace raw new int[KPAR] /
  delete[] startp with std::vector<int> startp(KPAR) and remove
  five blocks of commented-out debug output.

Misc dead-code / include cleanup
- parallel_reduce.h: remove the dead declaration
  bool check_if_equal(double& v) - never defined, never referenced
  anywhere in the repo.
- parallel_global.cpp: drop two unused includes (parallel_common.h,
  parallel_reduce.h) left over from earlier refactors.

Governance notes:
- GlobalV budget: PR total added=3 GlobalV refs, removed=19,
  net_delta = -16. The 3 new refs are inside the merged
  zpiece_distribute function (it uses the same GlobalV::MY_POOL etc.
  as the original two functions, they just appear on new lines in the
  diff). Remaining GlobalV usage in Parallel_Grid stays for Step 1
  (ProcessTopology injection).
- Added header includes: parallel_2d.h now includes <mpi.h> directly
  (it previously got MPI_Comm via parallel_comm.h); write_hs.h and
  hsolver_lrtd.hpp now include parallel_comm.h because the
  implementations reference DIAG_WORLD and POOL_WORLD respectively
  and were previously hiding that dependency behind the Parallel_Orbitals
  -> Parallel_2D -> parallel_comm transit.
- No INPUT / documentation change required: all public APIs keep the
  same signatures and semantics (bcast, grid reduce, Parallel_2D).
…ivide_mpi_groups tests

Scope: 5 files changed, +482 lines (all additions; zero behavior change
for existing code paths). No existing API touched.
Build: cmake --build build exit=0.
Test : OMP_NUM_THREADS=1 mpirun -np 4 MODULE_BASE_ProcessTopology ->
       7/7 tests passed (5x divide_mpi_groups + 2x ProcessTopology).

ProcessTopology (step 1a backbone, 规则1 / 规则2 / 规则3)
- New header-only + cpp class ProcessTopology in parallel_topology.h/.cpp:
  * Immutably holds the six communicators that currently live as raw
    globals (POOL_WORLD / KP_WORLD / INT_BGROUP / BP_WORLD /
    GRID_WORLD / DIAG_WORLD) together with kpar / my_pool /
    rank_in_pool / nproc_in_pool[...] / bndpar / my_bndgroup /
    rank_in_bgroup / nproc_in_bgroup / world size&rank.
  * Value semantics (trivially copyable ints + vector, no ref
    members, no mutable workflow switches) - addresses the two
    weaknesses observed in the old MPICommGroup (reference aliases
    between ngroups <-> nprocs_in_inter and lack of RAII).
  * Head-only-interface-minimal: includes only <vector> and <mpi.h>
    conditionally; does NOT include parallel_comm.h, parallel_global.h
    or any GlobalV header. All state flows in via constructor args,
    so the class is unit-testable in isolation and does not
    contribute to the 6-global-comm transit-include surface cleaned
    in step 0.
  * Default constructor produces a single-process fallback so any
    non-__MPI build path (including LCAO serial sections) can still
    take a `const ProcessTopology&` and work.
  * pool_root_rank(pool) helper computes the world rank of a pool
    root without reaching for GlobalV arrays - needed by both the
    upcoming Parallel_Kpoints migration and cube-output rank
    calculations.
- Base library wiring (CMakeLists rule, AGENTS.md deterministic-add
  requirement): add parallel_topology.cpp to the `base` OBJECT
  library source list directly after parallel_grid.cpp to keep the
  parallel_* cluster together. Nothing in the main build links the
  new object into paths that didn't already include `base`, so the
  incremental link cost is zero for unchanged modules.

Parallel_Global::divide_mpi_groups tests (第一次为纯算术核心补单测)
- New target MODULE_BASE_ProcessTopology in test_parallel/CMakeLists.txt,
  sources match ParaGlobal's transitive list plus parallel_topology.cpp.
- parallel_topology_test.cpp covers:
  * DivideMpiGroups.EvenDivision      8 proc / 4 pools even
  * DivideMpiGroups.UnevenDivision    5 proc / 2 pools (3+2)
  * DivideMpiGroups.ExactlyOneProcessPerGroup - N proc / N pools
  * DivideMpiGroups.OneGroup          5 proc / 1 pool
  * DivideMpiGroups.UnevenLargePools 24 proc / 5 pools (5+5+5+5+4)
  Each uses a local helper `divide_all` that enumerates every
  rank 0..procs-1 against divide_mpi_groups, then `validate_divide`
  asserts the group sizes sum to procs and every in-group rank is
  unique within its group. This directly guards against regressions
  in the upcoming `create_topology` factory because that factory is
  the one caller that stitches `divide_mpi_groups` outputs into a
  ProcessTopology object.
  * ProcessTopology.DefaultConstructorIsSingleProcess (non-MPI fallback)
  * ProcessTopology.ConstructAndAccessors - explicit 10-proc /
    KPAR=3 / BNDPAR=2 construction, plus a copy-semantics check.

Governance notes:
- GlobalV / PARAM / GlobalC budget: added lines = 0 reads of any
  global. The new class never includes global_variable.h or PARAM
  headers; the constructor takes everything as explicit args.
- Added includes in parallel_topology.h are both self-contained uses
  (<vector> is for member std::vector<int> nproc_in_pool_; <mpi.h>
  conditionally declares the MPI_Comm members). They do not leak
  declarations of the six global MPI_Comm variables - that surface
  remains isolated to parallel_comm.h.
- No documentation change because no external interface (INPUT,
  CLI, public class API used by non-parallel code) changed. The
  class is a new internal component used via its constructor and
  accessors; Parallel_Global::create_topology (next step) wires
  it at the startup boundary where call sites already take explicit
  parameters.
- C++11 compatible: using only std::vector, assert, int/long long
  POD types; no auto-return-type deduction, no std::move-only
  semantics required, no brace-init aggregates in new code beyond
  ProcessTopology copy test (already allowed in C++11).
- One variable per declaration: ProcessTopology members and test
  helper variables each get their own line; no comma-joined
  declarations introduced.
…tent `*_world_comm` naming

Scope: 3 files changed, +209 -82 lines.
Build: cmake --build build exit=0, abacus_basic_para linked successfully.
Test : OMP_NUM_THREADS=1 mpirun -np 4 MODULE_BASE_ProcessTopology
       -> 7 / 7 tests passed
       (5x divide_mpi_groups arithmetic
       + 2x ProcessTopology accessor / value-semantics cases).

Naming decision (after multi-round user review, naming principles:
  1. All 8 communicators share the uniform `_world_comm` suffix.
  2. The two legacy band-parallel domains INT_BGROUP / BP_WORLD are
     renamed directly after the "band-side vs k-side diff/same
     relation", ditching historically confusing abbreviations such
     as `intra / inter / BP / INT`.
  3. Two long-implicit domains are promoted to first-class names:
     the matrix 2D block-cyclic BLACS world and the atom 3D
     real-space DD (domain decomposition) world -- so callers no
     longer rely on the ad-hoc "pick POOL_WORLD or DIAG_WORLD or
     MPI_COMM_WORLD depending on the scene" convention.

Final 8-domain map (see parallel_topology.h for the full comment):
  Legacy global        -> New `*_world_comm` name            # 1-line semantic
  1. POOL_WORLD        -> pw_world_comm                     # Same-k, same-band-group PW tile (smallest parallel world)
  2. KP_WORLD          -> kmesh_world_comm                  # (user-suggested content name) k-mesh root bridge across pools
  3. INT_BGROUP        -> bsame_kdiff_world_comm            # (user-suggested abbreviation) band same, k different; same band-group union across k pools
  4. BP_WORLD          -> bdiff_ksame_world_comm            # (user-suggested abbreviation) band different, k same; intra-pool cross-band-group pair bridge
  5. GRID_WORLD        -> rgrid_world_comm                  # explicitly approved by user early on
  6. DIAG_WORLD        -> diag_world_comm                   # explicitly approved by user early on
  - (previously implicit)-> matrix_world_comm               # (user-suggested replacement for blacs_world) 2D block-cyclic matrix BLACS world
  - (previously implicit)-> atom_world_comm                 # (user-suggested) 3D real-space atomic DD / neighlist world

Scalar accessors aligned with the full-word `band_group`:
  - `bgroup` shorthand is expanded to the full `band_group`:
    my_band_group() / rank_in_band_group() / nproc_in_band_group().
  - `bndpar()` is kept because it matches the INPUT `BNDPAR` flag.
  - `kpar()` / `my_pool()` / `rank_in_pool()` / `nproc_in_pool()`
    are preserved to keep the naming compatible with the 100+
    occurrences of `pool` in Parallel_Kpoints and related modules.

New `band_group_root_rank(bg)` accessor:
  - Symmetric with `pool_root_rank(pool)` on the pool axis.
  - Constructor invariant: bndpar_ * nproc_in_band_group_ ==
    world_nproc_, so the root rank formula is simply
    `band_group * nproc_in_band_group_` -- this matches
    ABACUS divide_pools output exactly.
  - Tested on the 10-process / BNDPAR=2 / nproc_in_band_group=5
    case: ASSERT band_group_root_rank(0)==0 and
    band_group_root_rank(1)==5.

matrix_world_comm / atom_world_comm injection policy:
  - Both fields default to MPI_COMM_NULL in the constructor (two
    new trailing default parameters; the old 6-comm signature
    still works, so AGENTS rule deepmodeling#5 does not apply).
  - The "correct" BLACS / DD domain actually depends on the use
    case (LCAO diag / GK diag / MD step / ...). Distributed
    modules in Step 2 will fill these two handles in from the
    appropriate view on an as-needed basis; no real data flow is
    touched today.
  - Serial fallback default-constructor asserts matrix_world_comm
    == MPI_COMM_NULL and atom_world_comm == MPI_COMM_NULL. The
    ConstructAndAccessors test injects MPI_COMM_SELF /
    MPI_COMM_WORLD respectively and round-trips all 8 comm
    accessors plus copy semantics.

Governance (agent_governance_check.py --staged):
  - 1 warning only: "Documentation sync review". No user-visible
    INPUT / CLI / external API change, so documentation is
    correctly not updated. No exception needed.
  - GlobalV / PARAM / GlobalC: 0 new reads. All topology data
    are injected explicitly through constructor arguments, in
    line with AGENTS rule #1 (budget: non-increasing, no new
    globals introduced in this patch).
  - Header dependencies: only <vector> and <mpi.h> in
    parallel_topology.h, both required for self-containment
    (member type + MPI_Comm return types). The 6 legacy global
    communicators from parallel_comm.h are NOT transitively
    pulled in, satisfying rule deepmodeling#3.
  - C++11 compatible, one variable per declaration, no direct
    MPI calls in this patch.
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 1, 2026
abacus_fixer added 8 commits September 1, 2026 23:01
…ory + align error path

Scope: 9 files changed, +304 -6 lines.
Build: cmake --build build exit=0, abacus_basic_para linked successfully.
Test suite (all run with OMP_NUM_THREADS=1 mpirun -np 4):
  - MODULE_BASE_ProcessTopology : 8 / 8 passed
      (5 divide_mpi_groups arithmetic cases
       + 2 synthetic ProcessTopology accessor cases
       + 1 MPI-global integration case
         ParallelGlobalCreateTopology.FourRanksKpar2Bndpar2DiagNp2
         covering every scalar field, the 6 legacy-global comm sizes
         and matrix/atom == MPI_COMM_NULL).
  - MODULE_BASE_ParaReduce    : 10 / 10 passed
  - MODULE_BASE_ParaGlobal    : 6  / 6  passed
  - MODULE_BASE_ParaCommon    : 1  / 1  passed

1. Error-path fix in divide_mpi_groups:
   The even=true branch used to `exit(1)` on an uneven split, which
   bypassed ABACUS' WARNING_QUIT machinery (no stack cleanup, no
   consistent formatting). Replaced with
   ModuleBase::WARNING_QUIT("...Even partition requested...") so all
   failure paths now share the same error sink. No call-site change.

2. New Parallel_Global::create_topology(world, my_rank, kpar, bndpar,
   diag_np, grid_np):
   - Declared in parallel_global.h together with a detailed docstring
     explaining the kpar -> pool -> band-group layering, the legacy
     communicator aliases and the "caller later fills matrix / atom
     domains" injection contract.
   - Implemented in parallel_global.cpp under the same TU that owns
     divide_pools so we reuse every existing MPI_Comm_split /
     MPICommGroup::divide_group_comm helper rather than re-implement
     the partition.
   - non-__MPI builds return the trivial single-process
     ProcessTopology() instead of linking MPI code.
   - Snapshot construction:
       * nproc_in_pool vector is built with the even=false partition
         rule directly (base = world/kpar; first `extra_procs` groups
         get base+1). No MPI calls, O(kpar) only.
       * Legacy divide_pools(...) is called first -> fills POOL_WORLD
         / KP_WORLD / INT_BGROUP / BP_WORLD scalars and ints.
       * split_diag_world / split_grid_world(diag_np) are called
         immediately afterwards, folding the "two subroutines that
         real drivers have always called after divide_pools" step
         into a single factory so callers cannot forget to build
         rgrid/diag views. diag_np==0 safely falls back to diag_np=1.
       * Scalars (kpar/my_pool/rank_in_pool / band group triple) and
         all 6 legacy-global MPI_Comm handles plus MPI_COMM_NULL for
         matrix/atom are then forwarded to the ProcessTopology full
         constructor once.
   - Compatibility: after create_topology returns, the 6 extern
     MPI_Comm globals (POOL_WORLD..DIAG_WORLD) are still valid and
     hold exactly the same handles as before because they were the
     ones copied into the topology. No existing call site has to
     change today. The migration plan remains: new code takes a
     const ProcessTopology&; legacy code keeps reading the aliases.

3. Unit tests added to parallel_topology_test.cpp:
   - The file now provides its own main(argc, argv) that calls
     MPI_Init / MPI_Finalize around RUN_ALL_TESTS so gtest-based
     processes never issue MPI calls before MPI_Init (the error that
     surfaced while wiring the integration case).
   - Integration case ParallelGlobalCreateTopology.FourRanksKpar2Bndpar2DiagNp2:
       * GTEST_SKIP() if nproc != 4.
       * Asserts scalar invariants for every rank (world_size, kpar,
         bndpar, nproc_in_pool vector, pool_root_rank,
         band_group_root_rank).
       * Per-rank scalar expectations are laid out in a comment table
         R0..R3 that was cross-validated against the real factory
         output during development.
       * Asserts MPI_Comm_size/rank for all 6 derived legacy
         communicators plus matrix/atom == MPI_COMM_NULL.
   - Both synthetic ProcessTopology cases are untouched.

4. Hand-written SOURCES lists wired parallel_topology.cpp:
   create_topology lives in parallel_global.cpp and constructs a
   ProcessTopology, pulling the class' constructor symbol. Many unit
   test and module CMakeLists already enumerate parallel_global.cpp
   by hand and therefore need to also name parallel_topology.cpp as
   a local TU. The following targets were updated:
     - MODULE_BASE_ParaCommon / ParaGlobal / ParaReduce (the 3
       parallel unit tests that mirror ParaTopology).
     - MODULE_PW_pwdft tests (source_pw/module_pwdft/test).
     - MODULE_MD_func (source_md/test).
     - MODULE_IO_*  (source_io/test).
     - MODULE_PW_PW_Kernels_UTs (module_pw/kernels/test).
     - MODULE_CELL_ParaKpoints (source_cell/test).
   This is exactly the same repair we had to apply for ParaCommon in
   Step 0. AGENTS rule: "Keep source file additions deterministic;
   update the relevant CMakeLists.txt" – satisfied.

Governance notes (agent_governance_check --staged):
  - WARNING Header dependency (parallel_global.h:10 includes
    parallel_topology.h): Required because create_topology returns
    ProcessTopology by-value, which requires the complete class
    declaration in every TU that includes parallel_global.h.
    parallel_topology.h only brings in <vector> and <mpi.h>, so no
    parallel_comm.h 6-comm transitive leakage is reintroduced.
    Exception allowed = yes.
  - WARNING Documentation sync: 0 user-facing INPUT / CLI / external
    API change. Factory is not yet called from production code.
    Exception allowed = yes.
  - GlobalV / PARAM / GlobalC: 0 new reads. The factory only
    receives its inputs by-value from callers and constructs the
    topology object; it never reaches into PARAM. AGENTS rule 1
    budget strictly non-increasing.
  - AGENTS rule 5 (no new default args on existing interfaces):
    create_topology is a brand-new free function, so the 6-int + 8-MPI_Comm
    ProcessTopology constructor default-args for matrix_world / atom_world
    (introduced in step 1a) still apply, but we did not add defaults
    to an existing signature.
  - C++11 compatible. One variable per declaration. No new direct
    MPI calls outside the __MPI guarded factories.
…layout

The root of a band-group union is band_group * (nproc_in_pool[0] /
bndpar), not band_group * nproc_in_band_group: the two formulas only
coincide when kpar == 1. With asserts enabled (CI builds without
CMAKE_BUILD_TYPE) the wrong assert aborted MODULE_BASE_ProcessTopology,
while Release builds silently returned the wrong value.

Also replace the synthetic 10-rank layout in ConstructAndAccessors,
which violated the divide_pools constraint (BNDPAR>1 requires
NPROC % (BNDPAR*KPAR) == 0), with a valid 12-rank KPAR=3/BNDPAR=2
layout, and fix the 4-rank create_topology expectation
(band_group_root_rank(1) == 1 since bg1 = {1,3}).
…topology channel

Add 8 with_*_world_comm builders to ProcessTopology so callers can
derive solver-specific views (e.g. matrix/atom domain binding) without
mutating the shared world-level snapshot. Add a unit test verifying
each builder changes only its target domain and leaves all scalar
fields and other domain handles identical. Add a ProcessTopology topo_
member and set_topology() injection point to the ESolver base class.

No behavior change: the driver still uses the legacy split/init_pools
path; this is a pure channel addition for the upcoming migration.
Rename the class from ProcessTopology to ParallelPartition to avoid
the physics-specific meaning of "topology" in a condensed-matter code.
Also rename files via git mv (parallel_partition.h/cpp,
parallel_partition_test.cpp), the factory create_topology ->
create_partition, the test target MODULE_BASE_ProcessTopology ->
MODULE_BASE_ParallelPartition, and update all 7 referencing
CMakeLists.txt files. No behavior change.
Two bugs caused the np=4/kpar=2 SIGSEGV in the previous attempt:

1. Call order: the factory called divide_pools before split_diag_world
   / split_grid_world, while the legacy driver does the reverse. Restored
   the exact legacy order: split_diag_world -> split_grid_world ->
   divide_pools.

2. GlobalV write-back: the factory passed local int variables to
   divide_pools, so GlobalV::NPROC_IN_POOL / RANK_IN_POOL / MY_POOL /
   NPROC_IN_BNDGROUP / RANK_IN_BPGROUP / MY_BNDGROUP were never updated
   and stayed at 0, causing downstream consumers to read garbage. Now
   the factory passes the GlobalV references directly, exactly as the
   legacy init_pools did.

Also add the driver single-point switch to create_partition and the
ESolver topology injection (set_topology) that were reverted in the
previous step.

Verified: np=4 with kpar=1/2/4, bndpar=1, diago=1/2 all pass; 4 MPI
parallel unit tests all pass; 4-rank mpirun ParallelPartition 9/9.
Add 5 boundary test cases covering previously untested scenarios:

1. NonUniformKparPoolRootRank: 10 procs / kpar=3, pools {4,3,3}, verify
   pool_root_rank returns correct world ranks (0, 4, 7).
2. BndparKparCrossBandGroupRootRank: 12 procs / kpar=3 / bndpar=2,
   verify band_group_root_rank uses pool 0's slice formula
   (bg * nproc_in_pool[0] / bndpar), not bg * nproc_in_band_group.
3. OutOfRangeReturnsMinusOne: verify pool_root_rank / band_group_root_rank
   return -1 on bad indices; also added bounds check to
   nproc_in_pool(int pool) to return 0 instead of UB on out-of-range.
4. SerialFallbackDefaultConstructor: verify default-constructed
   ParallelPartition is a safe single-process trivial partition.
5. NonUniformKparSevenProcsThreePools: integration test for
   create_partition with 7 procs / kpar=3 -> pools {3,2,2}.

Total: 13/13 tests PASSED (4-rank mpirun).
…MPI builds

create_partition and divide_mpi_groups were swallowed by an outer
#ifdef __MPI block, so the serial fallback branch never compiled and
non-MPI linking failed with undefined reference to create_partition.
Close the guard after divide_pools and keep both functions outside;
divide_mpi_groups is pure arithmetic and is called unconditionally by
Parallel_K2D. Also refresh the stale availability note in the header.
Add parallel_partition.o to OBJS_PARALLEL so the Makefile-based build
links the ParallelPartition implementation, fixing undefined references
to ParallelPartition constructors in driver, esolver_fp, esolver_factory
and parallel_global.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant