[vsipl++] fftw3
Jules Bergmann
jules at codesourcery.com
Tue May 8 14:53:33 UTC 2007
Assem Salama wrote:
> Everyone,
> This patch address Jule's comments. Took out create_plan_defs.hpp and
> added a new file, fftw_support.hpp that contains overloaded functions
> for creating plans. Also, using Rt_tuple and Applied_layout in the
> create functions.
Assem,
This looks good. However, it looks like there are a few loose ends
that need fixing before checking in:
- you still include create_plan_defs (perhaps you have an old copy
in your SVN directory?)
- Although the split C->C Create_plan uses Applied_layout, some of
the other split Create_plan::create functions refer to
construct_dense_domain, which no longer exists. Can you check that
those are being exercised with the tests?
- A few naming nits.
Can you address these and send out another patch?
thanks,
-- Jules
> ------------------------------------------------------------------------
>
> Index: src/vsip/opt/fftw3/fft.cpp
> ===================================================================
> --- src/vsip/opt/fftw3/fft.cpp (revision 165174)
> +++ src/vsip/opt/fftw3/fft.cpp (working copy)
> @@ -18,6 +18,7 @@
> #include <vsip/core/config.hpp>
> #include <vsip/support.hpp>
> #include <fftw3.h>
> +#include <vsip/opt/fftw3/create_plan.hpp>
[1] Why does fft.cpp need to include create_plan.hpp?
Is it because it includes other header files that use create_plan.hpp?
In that case, it would better for those header files to directly include
create_plan.hpp.
Or is it because of some complexity in create_plan / create_plan_defs
/ fftw_support? If so, can you explain that again? :)
> Index: src/vsip/opt/fftw3/create_plan.hpp
> ===================================================================
> --- src/vsip/opt/fftw3/create_plan.hpp (revision 0)
> +++ src/vsip/opt/fftw3/create_plan.hpp (revision 0)
> @@ -0,0 +1,231 @@
> +/* Copyright (c) 2007 by CodeSourcery. All rights reserved.
> +
> + This file is available for license from CodeSourcery, Inc. under
the terms
> + of a commercial license and under the GPL. It is not part of the
VSIPL++
> + reference implementation and is not available under the BSD license.
> +*/
> +/** @file vsip/opt/fftw3/create_plan.hpp
> + @author Assem Salama
> + @date 2007-04-13
> + @brief VSIPL++ Library: File that has create_plan struct
> +*/
> +#ifndef VSIP_OPT_FFTW3_CREATE_PLAN_HPP
> +#define VSIP_OPT_FFTW3_CREATE_PLAN_HPP
> +
> +#include <vsip/dense.hpp>
> +
> +#include <vsip/opt/fftw3/fftw_support.hpp>
> +
> +namespace vsip
> +{
> +namespace impl
> +{
> +namespace fftw3
> +{
> +
> +// This is a helper strcut to create plans
[2] ^^^^^ Spelling
> +template<typename complex_type>
> +struct Create_plan;
> +
> +// interleaved
> +template<>
> +struct Create_plan<vsip::impl::Cmplx_inter_fmt>
> +{
> +
> + // create function for complex -> complex
> + template <typename plan_type, typename iodim_type,
> + typename T, dimension_type dim>
[3] Naming: to be consistent, template parameters should be
capitalized, with JavaStyle caps. I.e.
plan_type -> PlanT
iodim_type -> IodimT
dim -> Dim
Likewise below.
> + static plan_type
> + create(std::complex<T>* ptr1, std::complex<T>* ptr2,
> + int exp, int flags, Domain<dim> const& size)
> + {
> + int sz[dim],i;
> + for(i=0;i<dim;i++) sz[i] = size[i].size();
> + return create_fftw_plan(dim, sz, ptr1,ptr2,exp,flags);
> + }
> + static rt_complex_type const type = cmplx_inter_fmt;
[4] Please use a name other than 'type' for this member variable.
Perhaps 'format'?
In general, 'type' should be reserved for member type names create by
typedefs.
> +
> +};
> +
> +// split
> +template<>
> +struct Create_plan<vsip::impl::Cmplx_split_fmt>
> +{
> +
> + // create for complex -> complex
> + template <typename plan_type, typename iodim_type,
> + typename T, dimension_type dim>
> + static plan_type
> + create(std::pair<T*,T*> ptr1, std::pair<T*,T*> ptr2,
> + int exp, int flags, Domain<dim> const& size)
> + {
> + iodim_type iodims[dim];
> + int i;
> + Applied_layout<Layout<dim, typename Row_major<dim>::type,
> + Stride_unit_dense, Cmplx_split_fmt> >
> + app_layout(size);
> +
> + for(i=0;i<dim;i++)
> + {
> + iodims[i].n = app_layout.size(i);
> + iodims[i].is = iodims[i].os = app_layout.stride(i);
> + }
> +
> + return create_fftw_plan(dim, iodims, ptr1,ptr2, flags);
> +
> + }
> +
> + // create for real -> complex
> + template <typename plan_type, typename iodim_type,
> + typename T, dimension_type dim>
> + static plan_type
> + create(T *ptr1, std::pair<T*, T*> ptr2,
> + int A, int flags, Domain<dim> const& size)
> + {
> + iodim_type iodims[dim];
> + int i;
> + Domain<dim> dom = create_dense_domain(extent(size),
> + tuple_from_axis<dim>(A));
[5] dom is not used.
Also, since create_dense_domain is no longer defined, it suggests
that this create function is not being tested.
Can you check if any of the fft_be, fft, and fft_ext tests cover
real -> complex? I think fft_ext should cover this.
> + Applied_layout<Rt_layout<dim> >
> + app_layout(Rt_layout<dim>(stride_unit_align,
> + tuple_from_axis<dim>(A),
> + cmplx_split_fmt,
> + 0),
> + size, sizeof(T));
> +
> +
> + for(i=0;i<dim;i++)
> + {
> + iodims[i].n = app_layout.size(i);
> + iodims[i].is = iodims[i].os = app_layout.stride(i);
> + }
> +
> + return create_fftw_plan(dim, iodims, ptr1,ptr2, flags);
> + }
> +
> + // create for complex -> real
> + template <typename plan_type, typename iodim_type,
> + typename T, dimension_type dim>
> + static plan_type
> + create(std::pair<T*,T*> ptr1, T* ptr2,
> + int A, int flags, Domain<dim> const& size)
> + {
> + iodim_type iodims[dim];
> + int i;
[6] Does the Applied_layout object not work for complex -> real?
Likewise to above, since create_dense_domain is not defined, this
create function is not being exercised. Can you check if any of the
fft tests cover complex->real?
> + Domain<dim> dom = create_dense_domain(extent(size),
> + tuple_from_axis<dim>(A));
> +
> +
> + for(i=0;i<dim;i++)
> + {
> + iodims[i].n = dom[i].size();
> + iodims[i].is = iodims[i].os = dom[i].stride();
> + }
> +
> + return create_fftw_plan(dim, iodims, ptr1,ptr2, flags);
> + }
> +
> + static rt_complex_type const type = cmplx_split_fmt;
> +};
> +
> +
> +} // namespace vsip::impl::fftw3
> +} // namespace vsip::impl
> +} // namespace vsip
> +
> +#endif // VSIP_OPT_FFTW3_CREATE_PLAN_HPP
> Index: src/vsip/opt/fftw3/fftw_support.hpp
Looks good.
--
Jules Bergmann
CodeSourcery
jules at codesourcery.com
(650) 331-3385 x705
More information about the vsipl++
mailing list