Skip to content

refactor(rt): make block-cyclic layout access explicit via para_2d() - #8254

Open
kirk0830 wants to merge 3 commits into
deepmodeling:developfrom
kirk0830:pr/01-rt-explicit-para2d
Open

kirk0830 wants to merge 3 commits into
deepmodeling:developfrom
kirk0830:pr/01-rt-explicit-para2d

Conversation

@kirk0830

@kirk0830 kirk0830 commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Make all module_rt consumers request their block-cyclic matrix layout
explicitly through Parallel_Orbitals::para_2d() instead of relying on
inherited Parallel_2D members and implicit upcasts.

This is the first independently mergeable slice of the longer refactor that
will replace Parallel_2D inheritance with composition and, later, introduce
Matrix2d as the boundary for LAPACK/ScaLAPACK operations. Inheritance is
intentionally retained in this PR.

Motivation

Parallel_Orbitals currently exposes the layout of its base Parallel_2D
directly. RT code can therefore access nloc, desc, dim0, dim1, and
other distribution details without making the dependency visible.

That creates two problems:

  • it is unclear whether pv represents basis, wavefunction, or matrix layout;
  • removing the inheritance later would require changing all consumers at once.

This PR makes the dependency explicit and keeps the change behavior-neutral.

Changes

  • Add const and non-const para_2d() accessors.
  • Add get_blacs_ctxt() while the legacy getter family is still present.
  • Migrate module_rt call sites from inherited layout access to
    para_2d().
  • Update the corresponding RT tests mechanically.

Non-goals

  • This PR does not remove Parallel_2D inheritance.
  • It does not introduce BlockCyclicDistributionInformation or Matrix2d.
  • It does not change LAPACK/ScaLAPACK algorithms.
  • It does not change user-visible behavior or INPUT parameters.

Those changes are kept for subsequent PRs so that each consumer migration can
be reviewed and verified independently.

Scope

Only the module_rt call sites and the small compatibility accessor in
Parallel_Orbitals are changed.

Verification

  • git diff --check upstream/develop...HEAD: passed.
  • Agent governance using the branch merge-base: no deterministic errors;
    the only documentation warning is addressed by this no-docs-needed note.
  • Full CI build and regression tests are expected on this PR.

Follow-up

  1. Migrate the remaining LCAO consumers to explicit para_2d() access.
  2. Remove the implicit upcast and Parallel_2D inheritance after all consumers
    are explicit.
  3. Replace the legacy layout type with
    BlockCyclicDistributionInformation.
  4. Introduce Matrix2d/Matrix2dView as the math boundary.

Purely additive: no call site changes, inheritance still in place.
para_2d() (const + non-const) returns *this while Parallel_Orbitals derives
from Parallel_2D, giving the two escape hatches a const getter cannot serve
(LR_Util::setup_2d_division mutation, test-side descriptor writes).
get_blacs_ctxt() completes the inherited getter family, which covers the
other seven public data members.
@MrLi000001

Copy link
Copy Markdown
Collaborator

Thanks for slicing this refactor into a reviewable first step. Making the block-cyclic layout access explicit is the right direction, and keeping the change behavior-neutral while inheritance is retained is the correct sequencing. I verified the getter/member mapping (nloc -> get_local_size(), nrow -> get_row_size(), desc -> get_desc(), etc.) is one-to-one, and the test-side pv.set(...) -> pv.para_2d().set(...) substitutions are safe because Parallel_Orbitals does not override those methods. The non-cuBLASMp path is behavior-neutral.

Two things I'd like addressed before this merges.

  1. Title vs. content

This repo squash-merges, so the PR title becomes the permanent commit subject on develop. The current title, "replace Parallel_2D inheritance with composition", records a change this PR does not make (: public Parallel_2D is untouched, as the body itself states). After merge, git log will point people looking for the inheritance removal to a commit that did not remove it.

Suggested title:

plaintext

refactor(rt): make block-cyclic layout access explicit via para_2d()
2. The __CUBLASMP branches are not migrated

The body says "all module_rt consumers", but every #ifdef __CUBLASMP block still uses pv->nloc / pv->desc[i] / pv->dim0 directly:

band_energy.cpp 133-349
norm_psi.cpp 242-496
propagator_cn2.cpp 259-553
upsi.cpp 101-217
middle_hamilt.cpp 89-204
evolve_elec.cpp 47-49, 243-245
ENABLE_CUBLASMP defaults to OFF (CMakeLists.txt:97) and no CI job passes it, so these six regions are never compiled, meaning the green CI here does not cover them. When step 2 (removing the inheritance) lands, these blocks will fail to compile the first time someone enables cuBLASMp, months from now, with no indication why.

Please either migrate them here (it is the same mechanical substitution) or explicitly scope them out in the body and list these six ranges in the follow-up plan.

A few non-blocking suggestions:

Naming: para_2d is introduced here with zero existing callers, so it can be given its final name (get_block_cyclic_distribution()) for free now. Renaming later costs another 24-file diff.
Line length: 14 added lines exceed the 120-column limit (.clang-format is Microsoft style); e.g. the geadd calls in solve_propagation.cpp reach 147. Consider binding const Parallel_2D& p2d = pv.para_2d(); once per function.
Dead accessor: get_blacs_ctxt() has no caller in this PR and blacs_ctxt is already public, so suggest dropping it until needed.
Placeholder: the PR#TBD/3 comment should be a real issue/PR number or be removed.
Test backdoor: the non-const para_2d() lets tests write pv.para_2d().nloc = ... directly. A test-only helper would keep the seam from widening.
Out of scope (worth a separate issue, not this PR):

In norm_psi.cpp / middle_hamilt.cpp print_matrix blocks, the column-major stride uses get_col_size() where get_row_size() is expected; invisible under 1x1 grids.
ipiv is allocated as nrow + nb in propagator_cn2.cpp but nloc in propagator_taylor.cpp / solve_propagation.cpp; when ncol == 1 that undersizes the buffer.
Happy to re-review once the title and the cuBLASMp scope are addressed.

@kirk0830 kirk0830 changed the title refactor: replace Parallel_2D inheritance with composition refactor(rt): make block-cyclic layout access explicit via para_2d() Nov 1, 2026
@kirk0830

kirk0830 commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

@MrLi000001 thanks for your suggestion.

I have modified the title of this PR. For the cusolvermp side, my idea coincides mostly with your second suggested solution, that is, scoping it out. I will make my final decisions on this after a closer look and evaluating the radius of the explosion from the code changes.

Regards,
Yike

@AsTonyshment

Copy link
Copy Markdown
Collaborator

This is a very challenging and thankless task. I've thought about doing this myself, but the historical baggage of the ABACUS codebase has always been too heavy. I believe this would be a major step forward if the 2D block-cyclic distribution and its related logic could be handled in a cleaner and simpler way. (After all, I partly gave up on developing LCAO algorithms because of all this spaghetti code :( especially the 2D block-cyclic stuff)

As for the multi-GPU cuBLASMp/cuSOLVERMp path, that code was written in a rush and really should have been moved into a higher-level abstraction. Ideally, it should be unified with ScaLAPACK, since their programming models are essentially the same.🤔 (For example, we already have a unified abstraction layer with backend dispatch for single-process-CPU/single-GPU linear algebra libraries such as BLAS/LAPACK/cuBLAS/cuSOLVER) This would also make it easier to integrate other libraries like LibDDLA in the future.

Anyway, thanks for taking on all this work!

@AsTonyshment

Copy link
Copy Markdown
Collaborator

Oh and as for the multi-GPU tests, they can be triggered (hopefully) by /abacus-ci gpu

@AsTonyshment

Copy link
Copy Markdown
Collaborator

/abacus-ci gpu

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

GPU validation: failure

GPU cases: 97 passed, 2 failed, 1 infrastructure.

Open the Actions run | Download raw test files

Candidate: c1dc33fae882d4720ff50521be2e272ed13870f6

@AsTonyshment

Copy link
Copy Markdown
Collaborator

These two test fails are not related to this PR. GPU validation workflow might need an update (causing 97 passed, 2 failed, 1 infrastructure), and RTD build was killed due to unknown reason:
image

@Stardust0831

Copy link
Copy Markdown
Collaborator

These two test fails are not related to this PR. GPU validation workflow might need an update (causing 97 passed, 2 failed, 1 infrastructure), and RTD build was killed due to unknown reason: image

I checked the logs and reran the tests on SAI. The two PW failures are pre-existing Max_wfc_* comparison failures; scf_out_wf matches #8153 exactly. The infrastructure failure comes from the current CI configuration requesting 10_NO_MD_boundary_shift, which is absent from this PR’s older source tree. Running the PR executable against the latest test cases gives 95 passed, 5 failed, 0 infrastructure: the additional RT failures exercise fixes from #8207 and #8229 that this branch does not yet contain. All three RT failures also reproduce on the PR’s merge base with essentially identical results, so they were not introduced by this refactor. Updating the branch to current develop should bring the code and tests back into alignment. For RTD, the notification explicitly reports a 900-second timeout, with the log ending at the ABACUS compilation step.

@AsTonyshment

AsTonyshment commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

@Stardust0831 Okay I see, thanks for clarification. Btw, are you the only person who have permission on approving GitHub deployment?[doge] I found that I can't review my own deployment request🤔

@Stardust0831

Copy link
Copy Markdown
Collaborator

@Stardust0831 Okay I see, thanks for clarification. Btw, are you the only person who have permission on approving GitHub deployment?[doge] I found that I can't review my own deployment request🤔

The current reviewers for gpu-ci-manual are mohanchen, Critsium-xy, and me. Self-review is allowed, but your account is not on the reviewer list yet. I think adding you to the list would make sense. I’m also planning to further improve code isolation so GPU validation can become an optional check in the regular CI workflow.

An update on validation: I tested an isolated merge of this PR with the latest develop on SAI using the current test suite. The result is 98 passed, 2 failed, and 0 infrastructure errors. All 17 RT cases passed, and the two-node, 8-GPU cuSOLVERMp run completed successfully. The remaining two failures are the same PW Max_wfc failures seen in the develop nightly, including the known case documented in #8153.

Based on these results, I think we can consider the current refactor to have passed multi-GPU parallel testing

@kirk0830

kirk0830 commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

@AsTonyshment hi, thanks for your information, your suggestion is quite useful for me when handling the cusolvermp related codes.
The 2D-BCD (block cyclic distribution) related codes have blocked too many developers for years, I believe that both of us are not the first one who finds it should be refactored.
The major challenge I faced in the past was the laborious work demands, because the pv the instance of Parallel_Orbitals is used everywhere in ABACUS LCAO codes.
The second difficulty is, there should be much care be taken to encapsulate the 2D-BCD mechanism in a linalg container, delegate the call of ScaLAPACK/BLAS functions without any performance loss, it is hard if for a every general case without the advanced programming techniques (many C++ language features should be used).
Both can be hopefully solved now with AI-coding, because AI can generate code rapidly, I will standby to review, and itself is always a not-bad language lawyer so it can access to features more efficiently. What we should do now is to get the tests ready and I will take the responsibility to ensure the code quality in the aspect of design, naming, and so on :)

This branch has not been deployed

No deployments
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.

5 participants