On Tue, 05 Feb 2019 12:19:05 +0100
Ludovic Courtès <ludovic.courtes@inria.fr> wrote:

> Hello,
> 
> ericbavier@centurylink.net skribis:
> 
> > From: Eric Bavier <bavier@cray.com>
> >
> > * gnu/packages/maths.scm (elpa, elpa-openmpi): New variables.  
> 
> [...]
> 
> > +    (arguments
> > +     `(#:configure-flags
> > +       `(;"--enable-openmp"  
> 
> No OpenMP support, yet the ‘setup-tests’ phase sets OMP_NUM_THREADS.
> Did you mean to uncomment it?

Oops, right, I had meant to uncomment this.

> 
> > +         "--with-mpi=no"
> > +         ;; ELPA unfortunately does not support runtime dispatch, so we can
> > +         ;; only enable the "generic" kernels.  See the "Cross compilation"
> > +         ;; section of INSTALL.md.
> > +         "--enable-generic"
> > +         "--disable-sse" "--disable-sse-assembly" ;Require SSE3
> > +         "--disable-avx" "--disable-avx2" "--disable-avx512"  
> 
> For a future patch, do you think you can enable function
> multi-versioning (FMV) for this code using the scripts at
> <https://github.com/clearlinux/make-fmv-patch>?  That’d be pretty cool.

It would be cool.  And this package might be a good candidate to test,
since we can also compare the achieved performance against the
hand-rolled assembly.

> 
> > +    (synopsis "Eigenvalue solvers for symmetric matrices")
> > +    (description
> > +     "The ELPA library provides efficient and scalable direct eigensolvers for
> > +symmetric matrices.")
> > +    (license license:lgpl3)))  
> 
> v3 only?

Indeed.  There is no "or later" verbiage in file headers.

> 
> Also, should we add a ‘supported-systems’ field or is it supposed to
> work on any architecture?

It should work on any system, especially given that the "generic"
kernels are used.

> 
> Apart from that it LGTM, thank you!
> 

Thanks for the review.  I'll send an updated patch soon.

`~Eric