[Buildroot] [PATCH 2/3] package/fftw : Allow all precisions to be installed at the same time.
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Tue Aug 23 21:32:23 UTC 2016
Hello,
Why is this patch part of the patch series about adding support for
Cortex-A53 and other ARM related details? They should be independent.
Several other comments below. It's a pretty complicated change you are
doing here, it will most likely take several iterations to get
everything right and in good shape. Let me know if you need
help/assistance.
On Tue, 23 Aug 2016 10:53:22 +1000, Matt Flax wrote:
> This updates the current fftw package to allow the installation and selection
> of one more of the fftw precisions onto the system. Previously, you could
> only choose one prevision at a time, however there are many use cases where
> different precisions are required on the same system. Further, various packages
> selected BR2_PACKAGE_FFTW assuming the double precision version would be
> compile, which was a buggy assumption.
>
> Following previous suggestions and discussions, a common fftw mk file is used.
> The hash file is symbolically linked against. To preserve the previous behavior,
> if BR2_PACKAGE_FFTW is selected, this defaults to the double precision version.
> Other available precisions are now BR2_PACKAGE_FFTWF, BR2_PACKAGE_FFTWL,
> BR2_PACKAGE_FFTWQ which is the fftw standard for singe, long double and quad
> precisions.
I believe you should keep a top-level BR2_PACKAGE_FFTW option, and then
as sub-options have the four packages named:
fftw-double
fftw-single
fftw-quad
fftw-long-double
with the corresponding sub-options.
If you want to preserve backward compatibility, you can add:
config BR2_PACKAGE_FFTW
bool "fftw"
select BR2_PACKAGE_FFTW_DOUBLE if \
!BR2_PACKAGE_FFTW_SINGLE && \
!BR2_PACKAGE_FFTW_QUAD && \
!BR2_PACKAGE_FFTW_LONG_DOUBLE
> The following packages have been updated : liquid-dsp, gnuradio
> These packages required single precision installations.
> Other packages : libvips, pulseaudio, httping, imagemagick, alsa-utils
> implicitly required double precision without enforcing it, merely assuming
> this would happen by default. This was not the case when the user selected a
> different precision.
If they require double precision, then they should be updated to:
select BR2_PACKAGE_FFTW
select BR2_PACKAGE_FFTW_DOUBLE
and their <pkg>_DEPENDENCIES variable should be changed to depend on
fftw-double.
> package/fftw/Config.in | 69 +++++++---------------------------------
> package/fftw/fftw-common.mk | 44 +++++++++++++++++++++++++
> package/fftw/fftw.mk | 45 ++------------------------
In fact, put the contents of fftw-common.mk directly in fftw.mk.
> package/fftw/fftw/Config.in | 7 ++++
> package/fftw/fftw/fftw.hash | 1 +
> package/fftw/fftw/fftw.mk | 21 ++++++++++++
Having the naming proposed above will clarify things since we will no
longer have two fftw.mk files.
> config BR2_PACKAGE_FFTW_USE_SSE
> - bool
> + bool "use SSE"
> + depends on BR2_X86_CPU_HAS_SSE
>
> config BR2_PACKAGE_FFTW_USE_SSE2
> - bool
> + bool "use SSE2"
> + depends on BR2_X86_CPU_HAS_SSE
>
> config BR2_PACKAGE_FFTW_USE_NEON
> - bool
Please don't make this prompt options. Instead, add a first patch that
simply removes them, they are unnecessary. Just do:
ifeq ($(BR2_X86_CPU_HAS_SSE),y)
FFTW_CONF_OPTS += --enable-sse
else
FFTW_CONF_OPTS += --disable-sse
endif
ifeq ($(BR2_X86_CPU_HAS_SSE2),y)
FFTW_CONF_OPTS += --enable-sse2
else
FFTW_CONF_OPTS += --disable-sse2
endif
ifeq ($(BR2_ARM_CPU_HAS_NEON):$(BR2_ARM_SOFT_FLOAT),y:)
FFTW_CONF_OPTS += --enable-neon
FFTW_CFLAGS += -mfpu=neon
else
FFTW_CONF_OPTS += --disable-neon
endif
But please do this in a *separate* patch, prior to the fftw package
split.
> -config BR2_PACKAGE_FFTW_PRECISION_LONG_DOUBLE
> - bool "long double"
> - # long-double precision require long-double trigonometric routines
> - depends on !(BR2_TOOLCHAIN_BUILDROOT_UCLIBC && \
> - (BR2_arm || BR2_mips || BR2_mipsel))
Not your fault, but this BR2_TOOLCHAIN_BUILDROOT_UCLIBC dependency is
wrong: it should use BR2_TOOLCHAIN_USES_UCLIBC. I'll send a patch
fixing that, as it's also a bug on master.
> +FFTW_COMMON_CFLAGS = $(TARGET_CFLAGS)
> +ifeq ($(BR2_PACKAGE_FFTW_COMMON_FAST),y)
This option doesn't exist anywhere, it's still named
BR2_PACKAGE_FFTW_FAST in your code:
Config.in:config BR2_PACKAGE_FFTW_FAST
fftw-common.mk:ifeq ($(BR2_PACKAGE_FFTW_COMMON_FAST),y)
I think the name BR2_PACKAGE_FFTW_FAST is good.
> +# Generic optimisations
> +ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y)
> +FFTW_COMMON_CONF_OPTS += --enable-threads
> +FFTW_COMMON_CONF_OPTS += $(if $(BR2_GCC_ENABLE_OPENMP),--without,--with)-combined-threads
> +else
> +FFTW_COMMON_CONF_OPTS += --disable-threads
> +endif
> +FFTW_COMMON_CONF_OPTS += $(if $(BR2_GCC_ENABLE_OPENMP),--enable,--disable)-openmp
> +
> +FFTW_COMMON_CONF_OPTS += CFLAGS="$(FFTW_COMMON_CFLAGS)"
This last line is also wrong (but not your fault). Could you introduce
another preliminary patch that changes it to:
FFTW_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) $(FFTW_CFLAGS)"
that's the proper way of passing additional CFLAGS.
> diff --git a/package/fftw/fftw/Config.in b/package/fftw/fftw/Config.in
> new file mode 100644
> index 0000000..a91d519
> --- /dev/null
> +++ b/package/fftw/fftw/Config.in
> @@ -0,0 +1,7 @@
> +config BR2_PACKAGE_FFTW
> + bool "fftw"
> + select BR2_PACKAGE_FFTW_USE_SSE2 if BR2_X86_CPU_HAS_SSE2
> + select BR2_PACKAGE_FFTW_USE_NEON if BR2_ARM_CPU_HAS_NEON && !BR2_ARM_SOFT_FLOAT && (BR2_cortex_a53 || BR2_aarch64)
Please get rid of those selects, and don't handle the
Cortex-A53/AArch64 question in the same patch. This patch should *only*
do the package split. All other changes should be in separate patches
(but submitted all in the same series).
> +FFTW_VERSION = $(FFTW_COMMON_VERSION)
> +FFTW_SOURCE = fftw-$(FFTW_VERSION).tar.gz
> +FFTW_SITE = $(FFTW_COMMON_SITE)
> +FFTW_INSTALL_STAGING = $(FFTW_COMMON_INSTALL_STAGING)
> +FFTW_LICENSE = $(FFTW_COMMON_LICENSE)
> +FFTW_LICENSE_FILES = $(FFTW_COMMON_LICENSE_FILES)
> +
> +FFTW_CONF_ENV = $(FFTW_COMMON_CONF_ENV)
> +
> +FFTW_CONF_OPTS=$(subst enable-neon, disable-neon, $(FFTW_COMMON_CONF_OPTS))
Why are you doing this?
> +
> +FFTW_CFLAGS = $(FFTW_COMMON_CFLAGS)
This line has no effect.
> diff --git a/package/fftw/fftwf/Config.in b/package/fftw/fftwf/Config.in
> new file mode 100644
> index 0000000..1995fd1
> --- /dev/null
> +++ b/package/fftw/fftwf/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_FFTWF
> + bool "fftwf"
> + select BR2_PACKAGE_FFTW_USE_SSE if BR2_X86_CPU_HAS_SSE
> + select BR2_PACKAGE_FFTW_USE_SSE2 if BR2_X86_CPU_HAS_SSE2
> + select BR2_PACKAGE_FFTW_USE_NEON if BR2_ARM_CPU_HAS_NEON && !BR2_ARM_SOFT_FLOAT
Drop all those selects, see what I suggested above to handle
SSE/SSE2/NEON.
> +FFTWQ_CONF_OPTS=$(subst enable-neon, disable-neon, $(FFTW_COMMON_CONF_OPTS))
Why?
> diff --git a/package/gnuradio/Config.in b/package/gnuradio/Config.in
> index 50f5e7f..b07831d 100644
> --- a/package/gnuradio/Config.in
> +++ b/package/gnuradio/Config.in
> @@ -68,9 +68,9 @@ config BR2_PACKAGE_GNURADIO_UTILS
> Misc python utilities
>
> comment "gr-fft, -filter, -analog, -channels, -digital, -trellis, -pager, -qtgui depends fftw's single precision"
> - depends on !BR2_PACKAGE_FFTW_PRECISION_SINGLE
> + depends on !BR2_PACKAGE_FFTWF
We should now replace this dependency with a select. We had to use a
"depends on" because you can't select "choice" entries. Now that we
have separate packages for the various precisions, we should use this
capability and simplify things by using a select. So basically, remove
the if BR2_PACKAGE_FFTW_PRECISION_SINGLE condition, and make the
BR2_PACKAGE_GNURADIO_FFT option:
select BR2_PACKAGE_FFTW
select BR2_PACKAGE_FFTW_SINGLE
> # use FFTW instead of built-in FFT
> -ifeq ($(BR2_PACKAGE_FFTW_PRECISION_SINGLE),y)
> +ifeq ($(BR2_PACKAGE_FFTWF),y)
> LIQUID_DSP_LDFLAGS += -lfftw3f
> endif
>
> @@ -39,14 +39,18 @@ ifeq ($(BR2_powerpc)$(BR2_powerpc64),y)
> LIQUID_DSP_CONF_OPTS += --enable-simdoverride
> endif
>
> -ifeq ($(BR2_PACKAGE_FFTW_PRECISION_DOUBLE),y)
> +ifeq ($(BR2_PACKAGE_FFTW),y)
> LIQUID_DSP_LDFLAGS += -lfftw3
> endif
>
> -ifeq ($(BR2_PACKAGE_FFTW_PRECISION_LONG_DOUBLE),y)
> +ifeq ($(BR2_PACKAGE_FFTWL),y)
> LIQUID_DSP_LDFLAGS += -lfftw3l
> endif
>
> +ifeq ($(BR2_PACKAGE_FFTWQ),y)
> +LIQUID_DSP_LDFLAGS += -lfftw3q
> +endif
What happens if several fftw implementation are selected? We will pass
-lfftw3 -lfftw3f -lfftw3l -lfftw3q as LDFLAGS. Is this correct? Is this
useful? Should those case be mutually exclusive instead?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
More information about the buildroot
mailing list