[vsipl++] QR Solver

[Jules Bergmann]

Assem,

This looks good.  Can you address the comments below and then check
it in?

				thanks,
				-- Jules

Assem Salama wrote:
 > Everyone,
 >  This patch implements the QR backend using Cvsipl.
 >
 > Thanks,
 > Assem
 >
 >
 > ------------------------------------------------------------------------
 >
 > Index: src/vsip/core/cvsip/solver_qr.hpp
 > ===================================================================

 > +/// Qrd implementation using CVSIP
 > +
 > +/// Requires:
 > +///   T to be a value type supported by SAL's QR routines

[1] Change reference to C-VSIPL (instead of SAL).

 > +
 > +template <typename T,
 > +	  bool     Blocked>
 > +class Qrd_impl<T, Blocked, Cvsip_tag>
 > +{
 > +  typedef vsip::impl::dense_complex_type   complex_type;
 > +  typedef Layout<2, col2_type, Stride_unit_dense, complex_type> data_LP;

[2] Please change this to row2_type.

col2_type was necessary for Lapack.  For C-VISPL, either row2_type or
col2_type will work.  Since most user data will be row-major, using
row2_type will avoid the need for a transpose in the common case.

 > +  typedef Fast_block<2, T, data_LP> data_block_type;


 > +  // Member data.
 > +private:
 > +  typedef std::vector<T, Aligned_allocator<T> > vector_type;

[3] remove this (unused) typedef.

 > +
 > +  length_type  m_;			// Number of rows.
 > +  length_type  n_;			// Number of cols.
 > +  storage_type st_;			// Q storage type
 > +
 > +  Matrix<T, data_block_type> data_;	// Factorized QR(mxn) matrix
 > +  cvsip::Cvsip_matrix<T>     cvsip_data_;

[4] Please use cvsip::View<2, T, true> instead of Cvsip_matrix.

 > +  cvsip::Cvsip_qr<T>         cvsip_qr_;
 > +};



 > +#endif // VSIP_CORE_CVSIP_SOLVER_QR_HPP
 > Index: src/vsip/core/cvsip/cvsip.hpp
 > ===================================================================

 > +// some support defines

[5] First, anything we #define has to be prefixed with VSIP_ (if it
is in the spec) or VSIP_IMPL_ (if not).

Second, these following macros should be inline functions.  In C++
inline functions are just as efficient as macros, and they don't have
all the downsides.

 > +#define get_vsip_st(st) \
 > +	( (st == qrd_nosaveq)?     VSIP_QRD_NOSAVEQ: \
 > +	  (st == qrd_saveq1)?      VSIP_QRD_SAVEQ1: \
 > +	                           VSIP_QRD_SAVEQ \
 > +        )
 > +
 > +#define get_vsip_side(s) \
 > +	( (s == mat_lside)?        VSIP_MAT_LSIDE:VSIP_MAT_RSIDE )
 > +
 > +#define get_vsip_mat_op(op) \
 > +	( (op == mat_ntrans)?      VSIP_MAT_NTRANS: \
 > +	  (op == mat_trans)?       VSIP_MAT_TRANS: \
 > +	                           VSIP_MAT_HERM \
 > +	)
 > +
 >  }  // namespace cvsip
 >
 >  }  // namespace impl


 > Index: src/vsip/core/cvsip/cvsip_qr.hpp
 > ===================================================================
 > +template <typename T>
 > +class Cvsip_qr;

[6] Please remove this forward declaration.  It isn't necessary, since
the definition follows immediately.

 > +
 > +template <typename T>
 > +class Cvsip_qr : Non_copyable
 > +{
 > +  typedef typename Cvsip_traits<T>::qr_object_type     qr_object_type;
 > +
 > +  public:
 > +    Cvsip_qr<T>(int m, int n, vsip_qrd_qopt op);
 > +    ~Cvsip_qr<T>();

[7] Please name the constructor 'Cvsip_qr' instead of 'Cvsip_qr<T>'.
Likewise for the destructor.

This is consistent with all the other class definitions in the
library.  Technically, referring to the class as 'Cvsip_qr<T>' is OK.
However, this style will become burdensome for classes with many
template parameters.


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