[vsipl++] [patch] Use new dispatch for matvec functions

[Stefan Seefeld]

Don McCoy wrote:
> This patch has been expanded to include SAL and CML matvec functions and 
> it completely replaces previous versions.  It also incorporates the 
> feedback received from Stefan.  BLAS modifications will be posted in a 
> later patch.
> 
> Ok to commit?

Don, this looks good. I have a couple of minor issues, and one 
high-level question (to the group):

It is noteworthy that the C-VSIPL backends set ct_valid to false if the 
block types don't allow direct data access (i.e. Ext_data_cost > 0), yet 
for the reference implementation we call them explicitly, since the 
Ext_data handles the layout adjustments. Could you please add comments 
to point that out ?

> Index: src/vsip/core/dispatch.hpp
> ===================================================================
> --- src/vsip/core/dispatch.hpp	(revision 0)
> +++ src/vsip/core/dispatch.hpp	(revision 0)
> @@ -0,0 +1,71 @@
> +/* Copyright (c) 2008 by CodeSourcery.  All rights reserved. */
> +
> +/** @file    vsip/core/dispatch.hpp
> +    @author  Stefan Seefeld
> +    @date    2006-11-03
> +    @brief   VSIPL++ Library: Dispatcher harness.
> +*/
> +
> +#ifndef VSIP_CORE_DISPATCH_HPP
> +#define VSIP_CORE_DISPATCH_HPP
> +
> +
> +/***********************************************************************
> +  Declarations
> +***********************************************************************/
> +
> +namespace vsip
> +{
> +namespace impl
> +{
> +
> +// Operation Tags.
> +//
> +// Each operation (dot-product, matrix-matrix product, etc) has a 
> +// unique operation tag.
> +
> +struct Op_prod_vv_dot;    // vector-vector dot-product
> +struct Op_prod_vv_outer;  // vector-vector outer-product
> +struct Op_prod_mm;        // matrix-matrix product
> +struct Op_prod_mm_conj;   // matrix-matrix conjugate product
> +struct Op_prod_mv;        // matrix-vector product
> +struct Op_prod_vm;        // vector-matrix product
> +struct Op_prod_gemp;      // generalized matrix-matrix product
> +
> +
> +namespace dispatcher
> +{

I don't think these operation tags belong into this file, but I'm not 
sure where to put them instead. We (sort of) maintain a back-end 
repository in the vsip/core/impl_tags.hpp file. May be we could add 
operation tags there, too ?
Also, as all these tags are always and exclusively used in conjunction 
with the dispatcher, I'd suggest we put them into the dispatcher 
namespace, too, in an effort to clean up the vsip::impl namespace.
And, as the above tags are now (with your two patches) only used with 
the new dispatcher, this seems to be the right moment to do this.


> Index: src/vsip/core/matvec.hpp
> ===================================================================
> --- src/vsip/core/matvec.hpp	(revision 225932)
> +++ src/vsip/core/matvec.hpp	(working copy)
> @@ -19,8 +19,11 @@
>  #include <vsip/matrix.hpp>
>  #include <vsip/core/promote.hpp>
>  #include <vsip/core/fns_elementwise.hpp>
> +#include <vsip/core/general_evaluator.hpp>
> +#include <vsip/core/dispatch.hpp>
> +#include <vsip/core/impl_tags.hpp>
>  #ifndef VSIP_IMPL_REF_IMPL
> -# include <vsip/opt/general_dispatch.hpp>
> +# include <vsip/opt/dispatch.hpp>
>  # ifdef VSIP_IMPL_CBE_SDK
>  #  include <vsip/opt/cbe/cml/matvec.hpp>
>  # endif

I believe the general_evaluator.hpp file is obsolete (for the matvec 
operations) now, and thus shouldn't be included any longer.

> -// Dot product dispatch.  This is intentionally not named 'dot' to avoid
> -// conflicting with vsip::dot, which shares the same signature, forcing
> -// the user to resolve the call themselves.
> -
> +/// Dot product dispatch.  This is intentionally not named 'dot' to avoid
> +/// conflicting with vsip::dot, which shares the same signature, forcing
> +/// the user to resolve the call themselves.
>  template <typename T0, typename T1, typename Block0, typename Block1>
>  typename Promotion<T0, T1>::type
>  impl_dot(

Is this comment really meant to be put into the manual ? While I 
appreciate the comment as a hint for people working on the code, I don't 
think it belongs into documentation, and so should remain a simple '//' 
comment.

Thanks,
		Stefan

-- 
Stefan Seefeld
CodeSourcery
stefan at codesourcery.com
(650) 331-3385 x718