[vsipl++] [patch] Profiling for IIR, FIR and matrix-vector functions

[Jules Bergmann]

Don McCoy wrote:
 > This patch also reorganizes some of the description and operation
 > counting functions to one place and puts them under a namespace matching
 > their section name from the specification.  For example, 'dot', 'outer'
 > and other matrix-vector helper functions are under the 'impl::matvec'
 > namespace.  Signal processing helper functions, including the
 > Convolution and Correlation functions, are now under 'impl::signal'
 > namespace.
 >
 > This reorganization is helpful because it keeps all of these related
 > functions in one place, which should be easier for maintenance.  Note,
 > FFT helper functions and some of the operations counting functions have
 > not yet been moved either, pending approval of the current changes.
 >
 > Two miscellaneous fixes are included:  A change to the benchmarks
 > makefile skips building MPI benchmarks when not configured with MPI.
 > Second, a benchmark missed getting updated due to the change in location
 > of the ops_info.hpp header file.

Don,

This looks good.

I have a copule of comments below:

  - I think that 'impl::Length' would be more efficient than 'Domain'
    for passing view sizes to the Op_count_xyz::value() functions.

  - The template parameter to Domain is a 'dimension_type'.  To be correct,
    the template parameters for Op_count_xyz classes that take a dimension
    should also use 'dimension_type'.  (The same would be true if you switch
    to impl::Length).

Please have a look to see if those make sense.  Otherwise it looks
good, please check it in.

Also, I will rename the MPI specific benchmarks to use the same naming
convention as IPP and SAL specific benchmarks.

				-- Jules




[1] 'dimension_type' should be used for dimensions (such as D).
Likewise for several template declarations below.

 > +template <int D, typename T>
 > +struct Description
 > +{
 > +  static std::string tag(const char* op, length_type size)
 > +  {
 > +    std::ostringstream   st;
 > +    st << op << " " << Desc_datatype<T>::value() << " ";
 > +
 > +    st.width(7);
 > +    st << size;
 > +
 > +    return st.str();
 > +  }
 > +
 > +  static std::string tag(const char* op, Domain<D> const &dom_kernel,
 > +    Domain<D> const &dom_output)
 > +  {
 > +    std::ostringstream   st;
 > +    st << op << " "
 > +       << D << "D "
 > +       << Desc_datatype<T>::value() << " ";
 > +
 > +    st.width(4);
 > +    st << dom_kernel[0].size();
 > +    st.width(1);
 > +    st << "x" << (D == 2 ? dom_kernel[1].size() : 1) << " ";
 > +
 > +    st.width(7);
 > +    st << dom_output[0].size();
 > +    st.width(1);
 > +    st << "x" << (D == 2 ? dom_output[1].size() : 1);
 > +
 > +    return st.str();
 > +  }
 > +};
 > +
 > +} // namespace signal
 > +
 > +
 > +namespace matvec
 > +{
 > +template <typename T>
 > +struct Op_count_dot
 > +{
 > +  static length_type value(Domain<1> const &dom)

[2] Given the way these functions are called, it will probably be more
efficient to pass the size as a 'length_type' or a 'impl::Length'
instead of a 'Domain'.  A Domain encodes has offset and stride fields
that aren't used in the op-count calculation.

Because the Domain is being passed by reference, it is possible that
compiler could figure out that the offset and stride aren't used and
avoid creating them, but I don't think we can rely on that.

 > +  {
 > +    length_type count = dom[0].size() * Ops_info<T>::mul;
 > +    if ( dom[0].size() > 1 )
 > +      count += (dom[0].size() - 1) * Ops_info<T>::add;
 > +    return  count;
 > +  }
 > +};
 > @@ -545,18 +573,13 @@
 >    const_Vector<T0, Block0> v,
 >    const_Vector<T1, Block1> w) VSIP_NOTHROW
 >  {
 > -  typedef typename Promotion<T0, T1>::type return_type;
 > +  typedef typename Promotion<T0, T1>::type result_type;
 > +  Domain<1> dom_v( view_domain(v) );
 > +  impl::profile::Scope_event event(
 > +    impl::matvec::Description<result_type>::tag("dot", dom_v),
 > +    impl::matvec::Op_count_dot<result_type>::value(dom_v) );

[3] if you change the Op_count_dot::value to accept a Length, you can
use the 'extent()' function to get the size of the view as a Length.
This becomes:

   impl::profile::Scope_event event(
     impl::matvec::Description<result_type>::tag("dot", dom_v),
     impl::matvec::Op_count_dot<result_type>::value(extent(v)) );

 > Index: benchmarks/GNUmakefile.inc.in
 > ===================================================================
 > --- benchmarks/GNUmakefile.inc.in	(revision 145922)
 > +++ benchmarks/GNUmakefile.inc.in	(working copy)
 > @@ -41,6 +41,7 @@
 >                                           $(srcdir)/benchmarks/qrd.cpp
 >  benchmarks_cxx_srcs_ipp    := $(wildcard 
$(srcdir)/benchmarks/*_ipp.cpp)
 >  benchmarks_cxx_srcs_sal    := $(wildcard 
$(srcdir)/benchmarks/*_sal.cpp)
 > +benchmarks_cxx_srcs_mpi    := $(wildcard 
$(srcdir)/benchmarks/mpi_*.cpp)

[4] I will rename the mpi only benchmarks to match the convention.



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