-
Notifications
You must be signed in to change notification settings - Fork 578
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
MueLu: Add MagmaSparse support to matvec driver #4719
MueLu: Add MagmaSparse support to matvec driver #4719
Conversation
Fix C++ casting Fix missing reference in KK kernel Fix specialization to avoid Scalar test Add Magma Sparse as TPL in Trilinos Add Magma Sparse to Matvec driver Provide test for Magma Sparse cuda SpMV in driver
packages/muelu/cmake/Dependencies.cmake, line 9 at r1 (raw file):
Shouldn't this be an optional test dependency? |
packages/muelu/cmake/Dependencies.cmake, line 9 at r1 (raw file): Previously, jhux2 (Jonathan Hu) wrote…
Also, we've not done a good job of this, but could you also add this to the table in MueLu user's guide on page 19? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cgcgcg, @csiefer2, and @jjellio)
@jhu2 I didn't specify a test, because it wasn't obvious how to filter out tests by enabled execution spaces. Currently, this is tested with Cuda. Without out Cuda, it should build the original KK v Tpetra code. I haven't done the stuff to figure out Magma (on non-cuda). The code could function as a test though... For Magma and CuSparse TPL usage. As for documentation: What would I document? That you can access the TPL if you want? The same would be true for CuSPARSE. I haven't provided any TPL specific functionality in the library. |
|
||
TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES( MAGMASparse | ||
REQUIRED_HEADERS magmasparse.h | ||
REQUIRED_LIBS_NAMES magma_sparse magma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a TPL dependency? That is, does MAGMASparse depend on MAGMA, or is MAGMASparse stand-alone? Would it be possible to build with both MAGMASparse and MAGMA enabled, supposing that we had a MAGMA TPL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I started with MagmaSparse only... but they do depend. MagmaSparse requires Magma.
This TPL is a WIP. We could make a MAGMA TPL, that provides an identifier that states whether it provides sparse. I would need to look at NetCDF or HDF5, since those packages provide various attributes along with libraries/headers..
We should talk about my scheme for supporting multiple linear algebra things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TPL is a WIP.
Shall we mark it as such then? or do you mean "WIP" in the sense that you plan future work?
We should talk about my scheme for supporting multiple linear algebra things.
Let's do that at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP = work in progress = meaning the details of in CMake for the TPL need to be worked on - but that is in the direction of better multi-blas support.
MagmaSparse_SpmV_Pack (const crs_matrix_type& A, | ||
const vector_type& X, | ||
vector_type& Y) | ||
{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nicer for this class not to compile at all if Scalar is not one of the supported types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. This gets complicated, because we instantiate the driver itself for multiple ETI types. The general idea is that the code will compile, but that it simply won't provide the runtime option.
What we really need, is a better way to express the different instantiations (it would speed up build times).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Christian's way of doing it in kokkos-kernels makes sense. You have a struct with a static method that says "try using ${TPL}
to solve the problem." If ${TPL}
can't do it -- e.g., because it doesn't support the Scalar type -- the method returns false, and the caller has to try something else.
#include <magmasparse.h> | ||
template<typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Node> | ||
class MagmaSparse_SpmV_Pack { | ||
typedef Tpetra::CrsMatrix<Scalar,LocalOrdinal,GlobalOrdinal,Node> crs_matrix_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer C++11 using crs_matrix_type = Tpetra::CrsMatrix<...>;
.
No, I mean something different. I believe Magma should be in
Fair point. I guess the only thing one might document would be that one can do comparisons with Magma. |
I think, I maybe perhaps can possibly understand what you mean. With |
Yes, this is what I mean. |
You have increased my TRIBITS knowledge by 1! |
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: jjellio |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ csiefer2 ]! |
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
Merge on Pull Request# 4719: IS A SUCCESS - Pull Request successfully merged |
@trilinos/muelu
Description
This PR adds a MAGMA Sparse TPL to Trilinos and optionally enables that functionality in MueLu.
The MatvevDriver provides testing for correctness, but this testing is not enabled by default, since this TPL's use depends on how it was built (e.g., Cuda vs MIC).
The test currently provides Cuda functionality + testing. TODO is to ensure it works seamlessly with many core.
Notes:
This moves the matvec driver out of the EXPERIMENTAL guard (for evaluations we really don't want to be enabling Experimental IMO).
Motivation and Context
Allow testing of SpMV using various underlying implementations. This driver intentionally works at the Xpetra/Tpetra level, so that matrices are constructed in a manner similar to how MueLu does.
How Has This Been Tested?
The code has correctness testing, which is defined by comparing against SpMVs performed by Tpetra.
Checklist
Closes #4718