<div dir="ltr">On Sat, Mar 9, 2013 at 4:47 PM, Jed Brown <span dir="ltr"><<a href="mailto:jedbrown@mcs.anl.gov" target="_blank">jedbrown@mcs.anl.gov</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">Paul Mullowney <<a href="mailto:paulm@txcorp.com">paulm@txcorp.com</a>> writes:<br>
<br>
> Hi,<br>
><br>
> I've gotten GPU based icc preconditioners working with the following set<br>
> of patches. One uses ICC by loading an upper triangular matrix into the<br>
> cusparse class, and then setting the symmetry option, i.e.<br>
><br>
> ierr =<br>
> MatCreateSeqAIJCUSPARSE(comm,n,m,PETSC_NULL,num_entries_per_row,&A);CHKERRQ(ierr);<br>
> ierr = MatSetOption(A,MAT_SYMMETRIC,PETSC_TRUE);<br>
><br>
> I've organized the implementations into 2 files:<br>
> 1) icc-cholesky-cusparse-organization.patch : This patch has the basic<br>
> infrastructure for calling the symbolic and numeric factorizations (both<br>
> ICC and Cholesky). Currently these routines make scoping calls to the<br>
> implementations in aij/seq/aijfact.c<br>
<br>
</div>Can you write context like this into the commit messages? They are<br>
pretty opaque right now, but they are more likely to be read in the<br>
future than this email.<br>
<br>
This patch series does not have any tests. Can you add tests so that we<br>
can have a reasonable chance of not breaking it in the future?<br>
<br>
We can review on the mailing list, but I think we would prefer to use<br>
pull requests, mostly because comments there sort by context rather than<br>
by author. Barry, Matt, and others, do you prefer the granularity of<br>
email or bitbucket comments?<br></blockquote><div><br></div><div style>I can do either, but I think I prefer BB now.</div><div style><br></div><div style>I really do think everyone should put the component name at the start of</div>
<div style>every commit.</div><div style><br></div><div style> Mat</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
A few comments now:<br>
<br>
> exporting patch:<br>
<br>
This first line should not be in the patch file. It should start with the lines below:<br>
<br>
> # HG changeset patch<br>
> # User Paul Mullowney <<a href="mailto:paulm@txcorp.com">paulm@txcorp.com</a>><br>
> # Date 1362859983 25200<br>
> # Node ID 6be0d0f06ad18794377015b5ec468e11105b58b1<br>
> # Parent 6d60df3aadfe49f0bf640fa0a41fe3a423b88cf6<br>
> Implementation of ICC in this patch.<br>
<br>
Include the component (aijcusparse) in the commit message, otherwise we<br>
don't have a clue from the commit message what is actually being done.<br>
<br>
> diff -r 6d60df3aadfe -r 6be0d0f06ad1 src/mat/impls/aij/seq/seqcusparse/<a href="http://aijcusparse.cu" target="_blank">aijcusparse.cu</a><br>
> --- a/src/mat/impls/aij/seq/seqcusparse/<a href="http://aijcusparse.cu" target="_blank">aijcusparse.cu</a> Mon Feb 04 11:42:47 2013 -0700<br>
> +++ b/src/mat/impls/aij/seq/seqcusparse/<a href="http://aijcusparse.cu" target="_blank">aijcusparse.cu</a> Sat Mar 09 13:13:03 2013 -0700<br>
> @@ -1,12 +1,13 @@<br>
> /*<br>
> - Defines the basic matrix operations for the AIJ (compressed row)<br>
> + Defines the basic matrix operations for the AIJ (compressed row)<br>
> matrix storage format.<br>
> */<br>
><br>
> #include "petscconf.h"<br>
> PETSC_CUDA_EXTERN_C_BEGIN<br>
> #include "../src/mat/impls/aij/seq/aij.h" /*I "petscmat.h" I*/<br>
> -//#include "petscbt.h"<br>
> +#include <../src/mat/impls/sbaij/seq/sbaij.h><br>
> +<br>
> #include "../src/vec/vec/impls/dvecimpl.h"<br>
> #include "petsc-private/vecimpl.h"<br>
> PETSC_CUDA_EXTERN_C_END<br>
<br>
Not part of this patch, but PETSC_CUDA_EXTERN_C_BEGIN/END should be<br>
replaced with appropriate use of PETSC_EXTERN (as shown below). We are<br>
no longer mangling any public function names, so we can now compile<br>
PETSc using --with-clanguage=C++ and then call it from plain C.<br>
<br>
> @@ -53,12 +54,28 @@<br>
> EXTERN_C_BEGIN<br>
> extern PetscErrorCode MatGetFactor_seqaij_petsc(Mat,MatFactorType,Mat*);<br>
> EXTERN_C_END<br>
<br>
Use this instead:<br>
<br>
PETSC_EXTERN PetscErrorCode MatGetFactor_seqaij_petsc(Mat,MatFactorType,Mat*);<br>
<br>
> @@ -404,12 +417,119 @@<br>
><br>
><br>
><br>
> +<br>
> +<br>
<br>
Isn't this a lot of blank lines?<br>
<br>
> +#undef __FUNCT__<br>
> +#define __FUNCT__ "MatSeqAIJCUSPARSEBuildICCTriMatrices"<br>
> +PetscErrorCode MatSeqAIJCUSPARSEBuildICCTriMatrices(Mat A)<br>
<br>
Please use 'static' if the function is really only used in this one<br>
file. This identifier is more than 31 characters, thus too long for a<br>
public symbol.<br>
<br>
> +{<br>
> + Mat_SeqAIJ *a = (Mat_SeqAIJ*)A->data;<br>
> + Mat_SeqAIJCUSPARSETriFactors *cusparseTriFactors = (Mat_SeqAIJCUSPARSETriFactors*)A->spptr;<br>
> + GPU_Matrix_Ifc * cusparseMatLo = (GPU_Matrix_Ifc*)cusparseTriFactors->loTriFactorPtr;<br>
> + GPU_Matrix_Ifc * cusparseMatUp = (GPU_Matrix_Ifc*)cusparseTriFactors->upTriFactorPtr;<br>
<br>
Either line up consecutive lines or don't, but don't go half way. PETSc<br>
style guide says 'type *ptr', not 'type * ptr'.<br>
<br>
> + cusparseStatus_t stat;<br>
> + PetscErrorCode ierr;<br>
> + PetscInt *AiUp, *AjUp;<br>
> + PetscScalar *AAUp;<br>
> + PetscScalar *AALo;<br>
> + PetscInt nzUpper=a->nz,n=A->rmap->n,i,offset,nz,j;<br>
> + Mat_SeqSBAIJ *b = (Mat_SeqSBAIJ*)A->data;<br>
> + const PetscInt *ai=b->i,*aj=b->j,*vj;<br>
> + const MatScalar *aa=b->a,*v;<br>
<br>
Again, line up or don't line up.<br>
<br>
> + /* set this flag ... for performance logging */<br>
> + ((Mat_SeqAIJCUSPARSETriFactors*)A->spptr)->isSymmOrHerm = PETSC_TRUE;<br>
<br>
Does this have a MatSetOption() interface?<br>
<br>
> #undef __FUNCT__<br>
> #define __FUNCT__ "MatSeqAIJCUSPARSEICCAnalysisAndCopyToGPU"<br>
> PetscErrorCode MatSeqAIJCUSPARSEICCAnalysisAndCopyToGPU(Mat A)<br>
<br>
This can have static linkage, right?<br>
<br>
> @@ -706,8 +848,28 @@<br>
> ierr = MPI_Comm_size(PETSC_COMM_WORLD,&size);CHKERRQ(ierr);<br>
<br>
[Different patch] PETSC_COMM_WORLD cannot be used outside of global<br>
registration.<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
-- Norbert Wiener
</div></div>