Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScaLAPACK-like interface for Cholesky decomposition and eigensolver #886

Merged
merged 226 commits into from
Jul 28, 2023

Conversation

RMeli
Copy link
Member

@RMeli RMeli commented May 26, 2023

Introduce a ScaLAPACK-like interface to simplify adoption of DLA-Future, as a drop-in replacement.

This PR is based on work done to integrate DLA-Future in CP2K.

  • Introduce ScaLAPACK as optional dependency with -DDLAF_WITH_INTERFACE -DDLAF_WITH_C_API -DDLAF_WITH_SCALAPACK
    • MKL
    • Other
  • Introduce +interface +c +scalapack variant in Spack package
    • MKL
    • Other
  • Functions for pika and dlaf initialization/finalization
  • Functions for extracting Distribution, LayoutInfo, and CommunicationGrid from ScaLAPACK descriptors (and BLACS contexts)
  • Some BLACS functions
  • Interface for Cholesky decomposition
    • float
    • double
    • complex float
    • complex double
  • Interface for eigensolver
    • float
    • double
    • complex float
    • complex double
  • Use MPI_Thread_Init in MPI (non-pika) tests
  • Tests
    • Cholesky
    • Eigensolver
  • Documentation

This PR is very preliminary, there is still a lot of work and cleanup to do. This is especially true for the one test of the Cholesky decomposition interface, which are very messy and "hand-made" right now (quick-and-dirty solution to re-create something similar to what we have in CP2K).

I'm opening it as a draft PR early to facilitate discussion (about naming conventions, position within the code base, better design patterns, ...).

Early feedback is very welcome as usual.

Notes

  • [ ] @rasolca suggests to implement something similar to MatrixLocal AllGather(...) to re-use already implemented checks
    • Re-used DLA-Future functionality for creating the matrix and performing the tests (requires careful use of pika::resume and pika::suspend)

A lot of credit goes to @mtaillefumier and @msimberg, who worked on the initial integration of the Cholesky decomposition in CP2K.


Spack env:

spack:
  specs:
  - dla-future@develop +miniapps +cuda cuda_arch=89 ^intel-mkl threads=openmp ^mpich
    device=ch3 netmod=tcp ^[email protected] ^umpire~device_alloc
  view: false
  packages:
    all:
      variants:
      - cxxstd=17
  repos:
  - ~/git/DLA-Future/spack
  concretizer:
    unify: true

Build env:

spack -e ~/git/my-spack/envs/local/dlaf/mkl-mt-mpich-cuda build-env dla-future -- bash

CMake:

cmake -DDLAF_WITH_CUDA=on -DDLAF_WITH_INTERFACE=on -DDLAF_WITH_MKL=on -DMKL_LAPACK_TARGET=mkl::mkl_intel_32bit_omp_dyn -DMKL_SCALAPACK_TARGET=mkl::scalapack_mpich_intel_32bit_omp_dyn -DDLAF_MPI_PRESET=plain-mpi -DMPIEXEC_NUMCORES=24 -DCMAKE_EXPORT_COMPILE_COMMANDS=1   ..

Fix #610

@RMeli RMeli added the Type:New Feature New feature or request label May 26, 2023
@RMeli RMeli self-assigned this May 26, 2023
src/CMakeLists.txt Outdated Show resolved Hide resolved
@rasolca
Copy link
Collaborator

rasolca commented May 30, 2023

Just a few thoughts to start a discussion:

  1. I would use dlaf_C instead of interface
  2. Do we want to separate the C interface? Another option can be to add it directly in the tree.
    e.g.
    Currently
  • factorization.h
  • factorization
    • cholesky.h

with C wrappers

  • factorization.h
  • factorization_C.h
  • factorization
    • cholesky.h
    • cholesky_C.h
  1. We should avoid creating and destroying the Comm grid at each DLAF call. We should think of a way to reuse it.

  2. Some considerations about the descriptor:

  • In SCALAPACK the descriptor describes the full matrix, however ia, ja and n (I took Cholesky as example) describes the submatrix used in the algorithm. We should introduce a DLAF descriptor to describe the actual matrix used in the algorithm. It can be a simple struct with the following entries:
struct DLAF_descriptor {
Int m;
Int n;
Int mb;
Int nb;
Int isrc;
Int jsrc;
Int i; // currently not supported by DLAF (has to be 0, adding it so we can assert.) 
Int j; // same as i
Int ld;
};

(Note: which Int type?)

@RMeli
Copy link
Member Author

RMeli commented May 30, 2023

Thanks @rasolca for the comments.

  1. dlaf::dlaf_C sounds a bit repetitive. What about dlaf::C? Or did you mean dlaf_C as top-level namespace?
  2. Good point. Currently the ordering of the communicator grid is also hard-coded, which is a problem. What about having to create the grid manually (with utility functions), and then pass it to the interface? I just added something like pxpotrf(char uplo, T* a, int m, int n, int mb, int nb, int lld, const MPI_Comm& communicator, int nprow, int npcol) but we could do pxpotrf(char uplo, T* a, int m, int n, int mb, int nb, int lld, const CommGrid& comm_grid)?
  3. In combination with the above, we would have something like pxpotrf(char uplo, T* a, DLAF_descriptor desc, const CommunicatorGrid& comm_grid)? Can you please clarify what you have in mind for DLAF_descriptor?

@rasolca
Copy link
Collaborator

rasolca commented May 30, 2023

  1. C doesn't support namespaces. We should make sure that C headers do not contain any C++.
    My idea was to have include/dlaf and include/dlaf_C or have them in the same tree.
  2. const CommGrid& is C++. We should either use a pointer to the grid or store it in a global map and return an identifier (safer as we can check that the given key is available in the map).
  3. DLAF_descriptor should be a simple struct to avoid to pass all the parameters one by one. It also allows more code reuse as it can be initialized from the descriptor and only some extra parameters. I'm not sure if it is the best way to do it in C, so If someone has any better idea please comment.

Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Thanks @RMeli for taking this on, this looks already really good.

The main comment I have (as @rasolca) is about the naming. The C interface should be in the top-level namespace and I would name the functions dlaf_* (dlaf_C_* feels redundant since we're anyway talking about the C interface).

Regarding organization, a couple of other options are:

  • wrap all C++ functionality in #ifdef __cplusplus and leave the C interface outside the ifdef; this way we could have the C interface in the same header files
  • rename the C++ header files to use .hpp as the extension and provide a separate, but corresponding set of headers with .h extensions for the C interface

CMakeLists.txt Outdated Show resolved Hide resolved
include/dlaf/interface/blacs.h Outdated Show resolved Hide resolved
include/dlaf/interface/blacs.h Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
include/dlaf/interface/blacs.h Outdated Show resolved Hide resolved
include/dlaf/interface/blacs.h Outdated Show resolved Hide resolved
src/interface/cholesky.cpp Outdated Show resolved Hide resolved
include/dlaf/interface/cholesky.h Outdated Show resolved Hide resolved
include/dlaf/interface/cholesky.h Outdated Show resolved Hide resolved
test/unit/interface/test_cholesky.cpp Outdated Show resolved Hide resolved
@RMeli
Copy link
Member Author

RMeli commented Jul 18, 2023

cscs-ci run

Copy link

@mtaillefumier mtaillefumier left a comment

Choose a reason for hiding this comment

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

I think the code was scrutinized so many times over by the group that I skipped most of the cpp and c code.

Choose a reason for hiding this comment

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

we need to namespace the custom cmake modules with dlaf_ so that we do not enter in conflict with other findscalapack.cmake

@rasolca rasolca added this to the release v0.2.0 milestone Jul 21, 2023
@RMeli
Copy link
Member Author

RMeli commented Jul 24, 2023

Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

I have no further comments on this. Thanks @RMeli for all the work on this!

@rasolca
Copy link
Collaborator

rasolca commented Jul 28, 2023

cscs-ci run

Copy link
Collaborator

@rasolca rasolca left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@RMeli
Copy link
Member Author

RMeli commented Jul 28, 2023

Thanks you all for the extensive reviews and for all the suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High Type:New Feature New feature or request
Projects
Archived in project
5 participants