Sptrsv supernode #552

iyamazaki · 2019-12-19T22:37:45Z

This PR adds the options for supernodal-based sparse-triangular solve (with SuperLU or Cholmod) to the existing level-based scheduling sparse-triangular solve.

…SuperLU

…tor, give run-time optitions for merge, metis)

…supernodal block with zeros)

create sptrsv_aux.hpp for some common function between SuperLU and Cholmod

…_superlu tester NOTE: we store L in CSC (calling cusparse with UPPER+TRANSPOSE) and U in CSR (calling cusparse with UPPER+NoTRANSPOSE)

…od/superlu

ndellingwood · 2019-12-20T08:46:46Z

Running spot-check on kokkos-dev-2

ndellingwood · 2019-12-20T18:30:20Z

Spot-check on kokkos-dev-2 passes:

Running on machine: kokkos-dev-2
Going to test compilers:  gcc/7.3.0 gcc/9.1 intel/18.0.5 clang/8.0 cuda/10.1
Testing compiler gcc/7.3.0
Testing compiler gcc/9.1
  Starting job gcc-7.3.0-Pthread-release
  Starting job gcc-7.3.0-OpenMP-release
  PASSED gcc-7.3.0-Pthread-release
  Starting job gcc-9.1-OpenMP-release
  PASSED gcc-7.3.0-OpenMP-release
Testing compiler intel/18.0.5
  Starting job gcc-9.1-Serial-release
  PASSED gcc-9.1-OpenMP-release
Testing compiler clang/8.0
  Starting job intel-18.0.5-OpenMP-release
  PASSED gcc-9.1-Serial-release
  Starting job clang-8.0-Cuda_OpenMP-release
  PASSED intel-18.0.5-OpenMP-release
Testing compiler cuda/10.1
  Starting job clang-8.0-Pthread_Serial-release
  PASSED clang-8.0-Cuda_OpenMP-release
  PASSED clang-8.0-Pthread_Serial-release
  Starting job cuda-10.1-Cuda_OpenMP-release
  PASSED cuda-10.1-Cuda_OpenMP-release
#######################################################
PASSED TESTS
#######################################################
clang-8.0-Cuda_OpenMP-release build_time=605 run_time=518
clang-8.0-Pthread_Serial-release build_time=248 run_time=333
cuda-10.1-Cuda_OpenMP-release build_time=646 run_time=481
gcc-7.3.0-OpenMP-release build_time=235 run_time=177
gcc-7.3.0-Pthread-release build_time=215 run_time=176
gcc-9.1-OpenMP-release build_time=261 run_time=162
gcc-9.1-Serial-release build_time=238 run_time=181
intel-18.0.5-OpenMP-release build_time=441 run_time=160
#######################################################

mhoemmen

I didn't get through the last few files, but I think I said what I wanted to say. I didn't click "Request changes" because I don't want to block the scheduled kokkos-kernels release and promotion. Here are my main concerns about this PR:

It introduces new TPLs for only three functions that already exist in the BLAS and LAPACK TPLs. kokkos-kernels already has code for bringing in BLAS functions, that the author could have imitated.
The code only works for double and complex<double>. For other types, the code may run to completion but produce incorrect results with no diagnostic. kokkos-kernels functions should support at least the four types that the BLAS, LAPACK, cuSPARSE, etc. support, or stop with an error if they do not.
The code reimplements existing KokkosBlas::* functionality.

mhoemmen · 2019-12-20T17:41:51Z

Makefile.kokkos-kernels

@@ -24,6 +24,10 @@ KOKKOSKERNELS_INTERNAL_ENABLE_MKL := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE
 KOKKOSKERNELS_INTERNAL_ENABLE_CUSPARSE := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "cusparse" | wc -l))
 KOKKOSKERNELS_INTERNAL_ENABLE_CUBLAS := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "cublas" | wc -l))
 KOKKOSKERNELS_INTERNAL_ENABLE_MAGMA := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "magma" | wc -l))
+KOKKOSKERNELS_INTERNAL_ENABLE_CHOLMOD := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "cholmod" | wc -l))
+KOKKOSKERNELS_INTERNAL_ENABLE_SUPERLU := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "superlu" | wc -l))
+KOKKOSKERNELS_INTERNAL_ENABLE_LAPACKE := $(strip $(shell echo $(KOKKOSKERNELS_ENABLE_TPLS) | grep "lapacke" | wc -l))


@ndellingwood @iyamazaki just to summarize my concerns about adding CBLAS and LAPACKE TPLs in yesterday's phone call:

Trilinos historically let people add optional TPLs freely. However, as a practical matter, downstream users (like Clark) will not enable them unless they are enabled by default or unless they add a lot of benefit. PR tests do not enable CBLAS and LAPACKE, so you (either you personally, or kokkos-kernels) must do all the downstream testing yourself. Furthermore, you've also added work with no benefit in terms of features, since the CBLAS and LAPACKE features you use already exist in the BLAS and LAPACK.

I see this as a temporary solution, made necessary because of the release time frame. The long-term solution is to wrap the BLAS and LAPACK in kokkos-kernels, not to introduce new TPL dependencies. I don't mind temporary solutions at all and I'll be happy to help with long-term solutions, but please just be aware of the aforementioned concerns. I have no personal stake in this -- I'm just explaining the practical reality that new TPLs create a lot of trouble for downstream users.

mhoemmen · 2019-12-20T17:43:02Z

cmake/Dependencies.cmake

@@ -1,5 +1,5 @@
 TRIBITS_PACKAGE_DEFINE_DEPENDENCIES(
  LIB_REQUIRED_PACKAGES KokkosCore KokkosContainers KokkosAlgorithms 
-  LIB_OPTIONAL_TPLS quadmath MKL BLAS LAPACK CUSPARSE MAGMA
+  LIB_OPTIONAL_TPLS quadmath MKL BLAS LAPACK CUSPARSE MAGMA SUPERLU CHOLMOD LAPACKE CBLAS


Do we actually test the MAGMA TPL? Just curious -- I'm asking for another project.

Not that I know of.

We might have a bootstrap issue here for LAPACKE and CBLAS. The tpls don't exist in Trilinos (AFAIK) so adding the TPLs to Dependencies.cmake would also require adding Tribits find modules.

Trilinos does have find modules for SuperLU and Cholmod, but with different capitalization.
SUPERLU -> SuperLU and CHOLMOD -> Cholmod.

This problem of capitalize vs dont' capitalize is going to follow us forever...

@jjwilke I'm not sure how easy it would be to change these in a backwards-compatible way so that Trilinos could accept either capitalization, even for specific TPLs. @bartlettroscoe might have some ideas.

I'm not sure how easy it would be to change these in a backwards-compatible way so that Trilinos could accept either capitalization, even for specific TPLs.

This is an ugly problem. The names and capitalization of these TPLs should have been considered very carefully from the beginning. But with people breaking backward compatiblity left and right right now (because of the anticipated Trilinos 13.0 release) this is a great time to fix a bunch of these issues.

@jjwilke, can we get a list of TPLs defined in {{Trilinos/TPLsList.cmake}} that should be renamed or recapitalized to match standard CMake names for these TPLs?

I think this is why find_package() can also find <lower_case_name>_config.cmake filkes in addition to <UpperCaseName>Config.cmake files as this solves the capitalization problem, at least for installed packages that provide config.cmake files.

@bartlettroscoe wrote:

But with people breaking backward compatiblity left and right right now (because of the anticipated Trilinos 13.0 release) this is a great time to fix a bunch of these issues.

Is there a way to add "aliases" for TPL names? Could I set up a TPL Foo so that users could say TPL_ENABLE_Bar=ON and TPL_ENABLE_Foo=ON gets set?

Is there a way to add "aliases" for TPL names? Could I set up a TPL Foo so that users could say TPL_ENABLE_Bar=ON and TPL_ENABLE_Foo=ON gets set?

@mhoemmen, not sure. Would have to look at all of the use cases that would need to work (and it is easy to leave out use cases that don't occur to one right away).

@bartlettroscoe The main reason would be to support changes in capitalization. On the other hand, CMake already tells you about unused options, so perhaps that's good enough to support a change.

perf_test/sparse/KokkosSparse_sptrsv_aux.hpp

mhoemmen · 2019-12-20T17:45:30Z

perf_test/sparse/KokkosSparse_sptrsv_aux.hpp

+template<typename scalar_t>
+void forwardP_supernode(int n, int *perm_r, int nrhs, scalar_t *B, int ldb, scalar_t *X, int ldx) {
+
+  /* Permute right hand sides to form Pr*B */


This is almost certainly duplicated code. Many operations in kokkos-kernels and downstream code need to permute vectors or multivectors. If kokkos-kernels has an existing permutation function, please use it. Otherwise, consider adding one to kokkos-kernels.

mhoemmen · 2019-12-20T17:46:47Z

perf_test/sparse/KokkosSparse_sptrsv_aux.hpp

+}
+
+template<typename scalar_t>
+void backwardP_supernode(int n, int *perm_c, int nrhs, scalar_t *B, int ldb, scalar_t *X, int ldx) {


See above comment on permutations and duplicated code.

mhoemmen · 2019-12-20T18:27:23Z