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

Tpetra: Replace use of Tpetra::Details::BLAS with KokkosKernels #2850

Closed
kyungjoo-kim opened this issue May 30, 2018 · 11 comments
Closed

Tpetra: Replace use of Tpetra::Details::BLAS with KokkosKernels #2850

kyungjoo-kim opened this issue May 30, 2018 · 11 comments

Comments

@kyungjoo-kim
Copy link
Contributor

kyungjoo-kim commented May 30, 2018

Update Tpetra by replacing Tpetra::Details::BLAS with KokkosKernels.

@kyungjoo-kim kyungjoo-kim self-assigned this May 30, 2018
@kyungjoo-kim
Copy link
Contributor Author

kyungjoo-kim commented May 30, 2018

@mhoemmen

I searched tpetra directory of possible BLAS and LAPACK usages except for testing directories.

core/src/Tpetra_Details_gemv.hpp
core/src/Tpetra_Details_mklGemm.cpp
core/src/Tpetra_Details_cublasGemm.cpp
core/src/Tpetra_Details_libGemv.hpp
core/src/Tpetra_Details_libGemm.hpp
core/src/Tpetra_Details_gemm.hpp
core/src/Tpetra_Details_cublasGemv.hpp
**core/src/kokkos_refactor/Kokkos_MV_GEMM.hpp**
core/src/Tpetra_Details_Blas.hpp
core/src/Tpetra_Details_cublasGemv.cpp
core/src/Tpetra_Details_mklGemv.cpp
core/src/Tpetra_Details_libGemm.cpp
core/src/Tpetra_Details_cublasGemm.hpp
core/src/Tpetra_Details_libGemv.cpp

LAPACK is not used in Tpetra core directory but tsqr.

I am going to remove the above files and replace their usages with kokkoskernels. I guess that all we need is to change gemv and gemm (possibly axpy). Am I missing something ?

Questions:

  1. there is kokkos_refactor directory. do we keep the directory ?
  2. the above header files are not included any of tpetra core/src files. these detail files should not be used directly by users. thus, can we consider that we can remove them safely ?
  3. do we still need another level of wrapper to wrap kokkos kernels to insulate the interface ? this is a question for @trilinos/tpetra . We can remove all of the above files as they are not really used in tpetra core directory. If there is a code that uses tpetra detail blas, I can replace them with kokkoskernels. However, we can still keep the Tpetra_Details_libGemv(Gemm).hpp as internal tpetra interface and invoke kokkoskernels from the header. As kokkoskernels are fined-grained ETIed, we won't need cpp files to instanciate them.
  4. If we confirm that these detail files are indeed not used in tpetra, we can remove them. I don't think that application code uses tpetra blas interface for their dense problems. However, muelu or other trilinos packages might use this detail interface. @csiefer2 could you check if muelu uses the above tpetra interface or not ?

@mhoemmen mhoemmen changed the title Tpetra replaces Teuchos blas and lapack with KokkosKernels Tpetra: Replace use of Teuchos::BLAS and Teuchos::LAPACK with KokkosKernels May 31, 2018
@mhoemmen
Copy link
Contributor

@kyungjoo-kim wrote:

  1. there is kokkos_refactor directory. do we keep the directory ?

It was only ever used internally. If nothing in Trilinos uses it (look carefully in Stokhos), then we should delete it.

  1. ... these detail files should not be used directly by users. thus, can we consider that we can remove them safely ?

This is correct, as long as nothing in Trilinos currently uses them.

As kokkoskernels are fined-grained ETIed, we won't need cpp files to instanciate them.

The point of the .cpp files is NOT to do ETI. It's specifically to hide includes of third-party libraries from users' code. In particular, cuBLAS has two different APIs -- version 1 and version 2. Tpetra uses version 1; Sierra uses version 2. It's legal to use both APIs in the same code, but it is NOT legal to include both header files in the same code. I hid the cuBLAS header include in the .cpp file because Sierra's build broke when I didn't do that.

I will repeat this: We MUST hide TPL header includes in .cpp files.

@mhoemmen
Copy link
Contributor

One more thing: It is absolutely critical that GEMV and GEMM not default to some slow reference implementation for the four Scalar types that BLAS implementations normally support.

@mhoemmen
Copy link
Contributor

The reason I wrote the GEMV and GEMM wrappers in Tpetra was because kokkos-kernels didn't either GEMV or GEMM at the time. If kokkos-kernels still doesn't have GEMV or GEMM, then you can just copy and paste these into kokkos-kernels.

@mhoemmen
Copy link
Contributor

  1. do we still need another level of wrapper to wrap kokkos kernels to insulate the interface ?

No. The goal is for Tpetra to call kokkos-kernels for BLAS functions. Tpetra should call e.g., KokkosBlas::gemv.

@mhoemmen
Copy link
Contributor

@kyungjoo-kim wrote:

csiefer2 could you check if muelu uses the above tpetra interface or not ?

come on, you can grep a little bit

@aprokop
Copy link
Contributor

aprokop commented May 31, 2018

MueLu uses Teuchos::LAPACK.

@mhoemmen
Copy link
Contributor

mhoemmen commented Aug 7, 2018

Please take note of #3235 and its pending fix. Thanks!

@kyungjoo-kim
Copy link
Contributor Author

@aprokop This issue is restricted the replace Tpetra::Details::Blas with KokkosKernels::Blas. I do not want to replace all Teuchos::BLAS and LAPACK. Also KokkosKernels do not have LAPACK functionalities.

@mhoemmen The error reported from #3235 is blas library error check that we cannot handle. The solution should be to avoid zero dimensions when it calls BLAS functions. Instead of fixing Tpetra_Details_Blas.hpp (we are going to delete this anyway), let's fix the place where gemm is invoked to avoid zero dimensions. How do you like that ?

@kyungjoo-kim kyungjoo-kim changed the title Tpetra: Replace use of Teuchos::BLAS and Teuchos::LAPACK with KokkosKernels Tpetra: Replace use of Tpetra::Details::BLAS with KokkosKernels Aug 7, 2018
@kyungjoo-kim
Copy link
Contributor Author

@aprokop I am sorry that this issue title misleads you. I intended to replace Tpetra::Details::Blas.

@kyungjoo-kim
Copy link
Contributor Author

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants