Add cuDSS support via mfem::CuDSSSolver#717
Conversation
|
I fixed the formatting, but I'm not sure what to do about the other failures. The license check is failing because the test assumes the header will match exactly, but I added a new copyright line to two files. One possible solution would be to rewrite the test to check for the copyright notice and SPDX license identifier separately, but I think that's outside the scope of this PR. I think the other tests are failing because they're using the Spack route of installation, which I didn't update. It's not obvious to me that there's a way to make this work, besides waiting for the mfem Spack package to expose cuDSS support. |
hughcars
left a comment
There was a problem hiding this comment.
We'll take this for a spin, one small comment on the patching process but generally looks good.
hughcars
left a comment
There was a problem hiding this comment.
Hi @Pennycook,
I took a stab at cleaning things up as mentioned in the comments and there's a draft PR onto your fork at Pennycook#1. Most of that is noise from merging origin/main into your branch. If you merge into your fork it'll reduce down to a much smaller diff. The main thing was assembling the diffs.
cuDSS is a high-performance CUDA library for Direct Sparse Solvers. This commit enables the use of cuDSS in Palace by: - Wrapping mfem::CuDSSSolver in a new palace::CuDSSSolver; - Enabling the use of CuDSSSolver for KSP and WavePort; and - Adding cuDSS configuration options to CMake, for both Palace and MFEM.
Thanks again for this, @hughcars. I rebased my PR on top of origin/main and then cherry-picked these changes. I'm seeing some runtime failures locally that I didn't see before. Would you mind taking a look to see if the merged code looks as you expected? There were a few changes that didn't show up as merge conflicts (e.g., the JSON processing moved) that I had to fix, and so I might have missed something. |
|
Took cuDSS for a spin on GPU and benchmarked it against SuperLU_DIST. Short version: it's a solid win and the advantage grows with problem size. Setup
Results So ~3× faster on the coarse (direct) solve at the smaller size, growing to ~9× at the 869k-DOF coarse grid (5.25M-DOF fine problem), which is ~18% → ~46% off total wall time. cuDSS also appears to use less memory (2.8 vs 4.3 GB/rank), and both MaxIts=5 runs completed with no OOM. |
…solvers Follow-up to cuDSS support (#717). Two related build/recipe fixes kept out of the feature PR: - Vendor mfem PR #5389 (extern/patch/mfem/mfem_pr5389_cudss.diff, applied after mfem_pr5124_cudss.diff in both the CMake superbuild and the spack recipe): select the cuDSS communication layer matching the linked MPI so MPICH builds no longer abort at CuDSSSolver construction (cudssSetCommLayer returning CUDSS_STATUS_INVALID_VALUE). Remove once merged upstream and the pinned mfem is bumped. - Spack recipe: stop forcing +cuda on superlu-dist and strumpack. Palace does not use their GPU builds (strumpack.cpp calls DisableGPU(); SuperLU offload is off). Use mfem's conditional cuda_arch-propagation pattern instead, so superlu-dist defaults to its CPU build and strumpack works with whatever the mfem dependency provides. (Fixes the SuperLU device-alloc hang; the separate strumpack+cuda/libCEED crash is tracked in #803.)
…solvers Follow-up to cuDSS support (#717). Two related build/recipe fixes kept out of the feature PR: - Vendor mfem PR #5389 (extern/patch/mfem/mfem_pr5389_cudss.diff, applied after mfem_pr5124_cudss.diff in both the CMake superbuild and the spack recipe): select the cuDSS communication layer matching the linked MPI so MPICH builds no longer abort at CuDSSSolver construction (cudssSetCommLayer returning CUDSS_STATUS_INVALID_VALUE). Remove once merged upstream and the pinned mfem is bumped. - Spack recipe: stop forcing +cuda on superlu-dist and strumpack. Palace does not use their GPU builds (strumpack.cpp calls DisableGPU(); SuperLU offload is off). Use mfem's conditional cuda_arch-propagation pattern instead, so superlu-dist defaults to its CPU build and strumpack works with whatever the mfem dependency provides. (Fixes the SuperLU device-alloc hang; the separate strumpack+cuda/libCEED crash is tracked in #803.)
…solvers Follow-up to cuDSS support (#717). Two related build/recipe fixes kept out of the feature PR: - Vendor mfem PR #5389 (extern/patch/mfem/mfem_pr5389_cudss.diff, applied after mfem_pr5124_cudss.diff in both the CMake superbuild and the spack recipe): select the cuDSS communication layer matching the linked MPI so MPICH builds no longer abort at CuDSSSolver construction (cudssSetCommLayer returning CUDSS_STATUS_INVALID_VALUE). Remove once merged upstream and the pinned mfem is bumped. - Spack recipe: stop forcing +cuda on superlu-dist and strumpack. Palace does not use their GPU builds (strumpack.cpp calls DisableGPU(); SuperLU offload is off). Use mfem's conditional cuda_arch-propagation pattern instead, so superlu-dist defaults to its CPU build and strumpack works with whatever the mfem dependency provides. (Fixes the SuperLU device-alloc hang; the separate strumpack+cuda/libCEED crash is tracked in #803.)
…solvers Follow-up to cuDSS support (#717). Two related build/recipe fixes kept out of the feature PR: - Vendor mfem PR #5389 (extern/patch/mfem/mfem_pr5389_cudss.diff, applied after mfem_pr5124_cudss.diff in both the CMake superbuild and the spack recipe): select the cuDSS communication layer matching the linked MPI so MPICH builds no longer abort at CuDSSSolver construction (cudssSetCommLayer returning CUDSS_STATUS_INVALID_VALUE). Remove once merged upstream and the pinned mfem is bumped. - Spack recipe: stop forcing +cuda on superlu-dist and strumpack. Palace does not use their GPU builds (strumpack.cpp calls DisableGPU(); SuperLU offload is off). Use mfem's conditional cuda_arch-propagation pattern instead, so superlu-dist defaults to its CPU build and strumpack works with whatever the mfem dependency provides. (Fixes the SuperLU device-alloc hang; the separate strumpack+cuda/libCEED crash is tracked in #803.)
hughcars
left a comment
There was a problem hiding this comment.
Took it for a spin, worked great! There's an issue with building with mpich in mfem, but that will fail loudly and I have a patch on its way to mfem, so i think we can merge. I have one final commit on hughcars/pr717-cudss-followup with some small fix ups. If you cherry-pick that, rebase, and maybe bump the SchemaVer again (if another bump snuck in underneath), then we're good to go. Thank you for the submission! This'll be a meaningful improvement in nvidia gpu performance.
|
|
||
| #if defined(MFEM_USE_CUDSS) | ||
|
|
||
| #include "utils/iodata.hpp" |
There was a problem hiding this comment.
This file only uses MatrixSymmetry/SymbolicFactorization (defined in utils/labels.hpp) and nothing from iodata.hpp — the feedback commit switches this include to utils/labels.hpp, the actual definer, for include-what-you-use.
|
|
||
| } // namespace | ||
|
|
||
| CuDSSSolver::CuDSSSolver(MPI_Comm comm, MatrixSymmetry sym, SymbolicFactorization reorder, |
There was a problem hiding this comment.
cuDSS exposes only reordering reuse, not a strategy selection, so the SymbolicFactorization reorder arg is unused — the feedback commit marks it /*reorder*/ with a one-line note, kept only for parity with the other direct-solver wrappers.
| namespace palace | ||
| { | ||
|
|
||
| class IoData; |
There was a problem hiding this comment.
Unused IoData forward-decl — the feedback commit removes it.
| #include "linalg/ams.hpp" | ||
| #include "linalg/arpack.hpp" | ||
| #include "linalg/blockprecond.hpp" | ||
| #include "linalg/cudss.hpp" |
There was a problem hiding this comment.
linalg/cudss.hpp isn't used in this file — the feedback commit removes it.
| pc = MakeWrapperSolver<OperType, CuDSSSolver>(linear, comm, pc_mat_sym, | ||
| linear.sym_factorization, | ||
| linear.reorder_reuse, print); | ||
| #else |
There was a problem hiding this comment.
The feedback commit drops this #else MFEM_ABORT so the case matches the empty-guarded MUMPS one; the single "not built with cuDSS" check moves to an MFEM_VERIFY in iodata.cpp alongside the SuperLU/STRUMPACK/MUMPS guards.
| "description": "Use the Jacobi preconditioner." | ||
| }, | ||
| { | ||
| "const": "cuDSS", |
There was a problem hiding this comment.
New allowed enum value ⇒ backward-compatible SchemaVer REVISION. main is now at 1-1-0 (RationalImpedance, #770), so the feedback commit bumps the schema $id 1-1-0 → 1-2-0 and tags the CHANGELOG entry SchemaVer 1-2-0 (check-schema-version enforces this). You might need to bump it further if something else gets in first.
cuDSS is a high-performance CUDA library for Direct Sparse Solvers.
This commit enables the use of cuDSS in Palace by:
mfem::CuDSSSolverin a newpalace::CuDSSSolver;CuDSSSolverfor KSP and WavePort; andI've opened this as a draft for now because it depends on mfem/mfem#5124, which has not yet been merged.
An example of building with cuDSS: