[vsipl++] CLAPACK

[Jules Bergmann]

Assem Salama wrote:
> Everyone,
>   I have changed modules file to now only load SRC dir of clapack. I 
> have also added an option to configure to allow a user to specify CFLAGS 
> for building clapack. The option is --with-clapack-cflags. It seams like 
> the option -funroll-all-loops works well with clapack so this is 
> something the user might want to specify. If this option is not used, 
> the default CFLAGS is used.
> 
> Thanks,
> Assem Salama

Assem,

Looks good.  Please address the comments below and then check it in.

				thanks,
				-- Jules


> 
> 
> ------------------------------------------------------------------------
> 
> Index: configure.ac
> ===================================================================
> RCS file: /home/cvs/Repository/vpp/configure.ac,v
> retrieving revision 1.88
> diff -u -r1.88 configure.ac
> --- configure.ac	21 Mar 2006 15:52:23 -0000	1.88
> +++ configure.ac	22 Mar 2006 00:05:39 -0000
> @@ -170,6 +170,21 @@
>  
>  
>  # LAPACK and related libraries (Intel MKL)
> +
> +# this option allows the user to OVERRIDE the default CFLAGS for CLAPACK
> +# it seams that when -funroll-all-loops is specified, it runs a little better
> +# it is up to the user to try specifying his own set of CFLAGS. If this option
> +# is not used, CLAPACK_CLFAGS defaults to CFLAGS. .in files will find this
> +# value in CLAPACK_CFLAGS

This comment says a little too much.  We're not really sure if 
-funroll-all-loops will make a noticeable performance difference (it may 
be that the longest running routines are all in ATLAS which this 
wouldn't affect, i.e. amdahls law).

How about:

This option allows the user to OVERRIDE the default CFLAGS for CLAPACK. 
  If this option is not used, CLAPACK_CLFAGS defaults to CFLAGS. .in 
files will find this value in CLAPACK_CFLAGS.


Also, don't forget punctuation, grammer, etc.  In general, comments 
should be grammatically correct sentences.  It makes them easier to read 
and understand.



> +AC_ARG_WITH(clapack-cflags,
> +  AS_HELP_STRING([--with-clapack-cflags=CLAPACK_CFLAGS],
> +                 [Specify CFLAGS to use when building builtin clapack.
> +		  Only used if --with-lapack=builtin.]),
> +            CLAPACK_CFLAGS=$withval,
> +            CLAPACK_CFLAGS=no)


We have grouped the argument processing in configure.ac seperately from 
the logic.  Its not strictly necessary to do this, but it makes finding 
things in configure.ac easier.  Also, it is good to keep related logic 
together.

You should move this AC_SUBST line to --->

> +# let's not forget AC_SUBST!
> +AC_SUBST(CLAPACK_CFLAGS)
> +
>  AC_ARG_WITH([lapack],
>    AS_HELP_STRING([--with-lapack\[=PKG\]],
>                   [enable use of LAPACK if found
> @@ -317,6 +332,11 @@
>  AC_SUBST(CXXDEP)
>  AC_LANG(C++)
>  
> +# assign cflags to CLAPACK_CFLAGS if the user didn't use --with-clapack-cflags
> +if test "$CLAPACK_CFLAGS" == "no"; then
                              ^^
Use "=" instead of "==".  It is more portable.


> +  CLAPACK_CFLAGS=$CFLAGS
> +fi

---> move AC_SUBST line here

> +
>  AC_MSG_CHECKING([for FORTRAN float return type])
>  if test "$host_cpu" == "x86_64"; then
>    AC_DEFINE_UNQUOTED(VSIP_IMPL_FORTRAN_FLOAT_RETURN, double,
> Index: vendor/clapack/SRC/make.inc.in
> ===================================================================
> RCS file: /home/cvs/Repository/clapack/SRC/make.inc.in,v
> retrieving revision 1.1
> diff -u -r1.1 make.inc.in
> --- vendor/clapack/SRC/make.inc.in	21 Mar 2006 21:38:49 -0000	1.1
> +++ vendor/clapack/SRC/make.inc.in	22 Mar 2006 00:05:40 -0000
> @@ -19,9 +19,12 @@
>  #  selected.  Define LOADER and LOADOPTS to refer to the loader and
>  #  desired load options for your machine.
>  #

Likewise, it is not necessary to mention that -funroll-all-loops might 
work well in this comment.

> -# configure will now substitute correct values for these variables
> +# configure will now substitute correct values for these variables.
> +# we added a variable called CLAPACK_CFLAGS that will allow someone to 
> +# specify special flags that could make CLAPACK run faster. It seams
> +# like -funroll-all-loops works very well.
>  CC        = @CC@
> -CFLAGS    = @CFLAGS@
> +CFLAGS    = @CLAPACK_CFLAGS@
>  LOADER    = $(CC)
>  LOADOPTS  = $(CFLAGS)
>  NOOPT     = 


-- 
Jules Bergmann
CodeSourcery
jules at codesourcery.com
(650) 331-3385 x705