| Message ID | 20210724214526.47637-3-arnout@mind.be |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/3] package/clapack: introduce BR2_PACKAGE_CLAPACK_ARCH_SUPPORTS | expand |
Arnout, All, On 2021-07-24 23:45 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly: > From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > > armadillo can use clapack, lapack or openblas as BLAS provider and > clapack or lapack as LAPACK provider. > > This patch > - adds hidden variable to check dependencies/requirement for each of them > - adds an option to use openblas as BLAS provider > > The choice is required since applications may potentially need > lapack/clapack. > > Note that a choice between lapack and clapack is hard to do in Kconfig, > because it inevitably leads to a circular dependency. > > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > Changes v3 -> v4: [Done by Arnout] > - Split off lapack/clapack arch depends in separate patches > - Simplify the comments (no need for the powerpc complexity) > - Remove the choices because clapack/lapack choice doesn't work. Keep a > single config for openblas. > Changes v2 -> v3: > - drop all default statements for choice (Thomas) > - add explicit -l since blas libary is called liblas for (c)lapack and > libopenblas for openblas (Thomas) > - add a choice for lapack selection between lapack, clapack or none > Changes v1 -> v2: > - add openblas as blas provider > --- > package/armadillo/Config.in | 32 +++++++++++++++++++++++--------- > package/armadillo/armadillo.mk | 28 +++++++++++++++++++++++++++- > 2 files changed, 50 insertions(+), 10 deletions(-) > > diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in > index b2b61a3233..710da0314f 100644 > --- a/package/armadillo/Config.in > +++ b/package/armadillo/Config.in > @@ -1,20 +1,34 @@ > +config BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS > + bool > + default y if BR2_PACKAGE_CLAPACK_ARCH_SUPPORTS > + default y if BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN > + default y if BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > + > comment "armadillo needs a toolchain w/ C++" > + depends on BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS > depends on !BR2_INSTALL_LIBSTDCPP > - depends on !BR2_powerpc > - depends on !BR2_m68k_cf > - > -comment "armadillo needs a glibc toolchain w/ C++" > - depends on BR2_powerpc > - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC > > config BR2_PACKAGE_ARMADILLO > bool "armadillo" > + depends on BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS > depends on BR2_INSTALL_LIBSTDCPP > - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack > - depends on !BR2_m68k_cf # clapack > - select BR2_PACKAGE_CLAPACK > + select BR2_PACKAGE_CLAPACK if !BR2_PACKAGE_LAPACK && !BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS > help > Armadillo: An Open Source C++ Linear Algebra Library for > Fast Prototyping and Computationally Intensive Experiments. > > http://arma.sourceforge.net/ > + > +if BR2_PACKAGE_ARMADILLO > + > +config BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS Note the option name here ^^^ [...] > + bool "use openblas" > + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > + select BR2_PACKAGE_OPENBLAS > + help > + Use OpenBLAS as BLAS library. Without this option, clapack or lapack > + will be used. > + > +endchoice > + > +endif > diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk > index 624b842ef6..82df7602be 100644 > --- a/package/armadillo/armadillo.mk > +++ b/package/armadillo/armadillo.mk > @@ -7,11 +7,37 @@ > ARMADILLO_VERSION = 9.900.2 > ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz > ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma > -ARMADILLO_DEPENDENCIES = clapack > ARMADILLO_INSTALL_STAGING = YES > ARMADILLO_LICENSE = Apache-2.0 > ARMADILLO_LICENSE_FILES = LICENSE.txt > > ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false > > +# blas support may be provided by lapack, clapack or openblas > +# blas library from (c)lapack is libblas.a, libopenblas.a otherwise > +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON > +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) [...] so I guess you meant BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS here, no? > +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas > +ARMADILLO_DEPENDENCIES = openblas > +else > +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas > +ifeq ($(BR2_PACKAGE_CLAPACK), y) We don't usually add a space after the comma in an ifeq, especially in cases like this simple test. Why duplicate the dependency on lapack/clapack in the !openblas case, when the same dependencies already exist, below? I mean: in the !openblas case, we know that either lapack or clapack are enabled, so we know will hit either case in the block [...] > +ARMADILLO_DEPENDENCIES = clapack > +else > +ARMADILLO_DEPENDENCIES = lapack > +endif > +endif [...] here: > +# lapack support may be provided by lapack or clapack > +# but not by openblas > +ifeq ($(BR2_PACKAGE_CLAPACK),y) > +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON > +ARMADILLO_DEPENDENCIES += clapack > +else ifeq ($(BR2_PACKAGE_LAPACK),y) > +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON > +ARMADILLO_DEPENDENCIES += lapack > +else > +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=OFF > +endif Additionally, this block will be hit even in the openblas case. Is this expected? Regards, Yann E. MORIN. > $(eval $(cmake-package)) > -- > 2.31.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 25/07/2021 09:57, Yann E. MORIN wrote: > Arnout, All, > > On 2021-07-24 23:45 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly: >> From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> [snip] >> +config BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS > > Note the option name here ^^^ [...] > >> + bool "use openblas" >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS >> + select BR2_PACKAGE_OPENBLAS >> + help >> + Use OpenBLAS as BLAS library. Without this option, clapack or lapack >> + will be used. >> + >> +endchoice >> + >> +endif >> diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk >> index 624b842ef6..82df7602be 100644 >> --- a/package/armadillo/armadillo.mk >> +++ b/package/armadillo/armadillo.mk >> @@ -7,11 +7,37 @@ >> ARMADILLO_VERSION = 9.900.2 >> ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz >> ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma >> -ARMADILLO_DEPENDENCIES = clapack >> ARMADILLO_INSTALL_STAGING = YES >> ARMADILLO_LICENSE = Apache-2.0 >> ARMADILLO_LICENSE_FILES = LICENSE.txt >> >> ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false >> >> +# blas support may be provided by lapack, clapack or openblas >> +# blas library from (c)lapack is libblas.a, libopenblas.a otherwise >> +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON >> +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) > > [...] so I guess you meant BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS here, no? See, that's why I resent this patch rather than just applying it :-) TBH I meant BR2_PACKAGE_ARMADILLO_OPENBLAS in Config.in, but it doesn't matter much. > >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas >> +ARMADILLO_DEPENDENCIES = openblas >> +else >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas >> +ifeq ($(BR2_PACKAGE_CLAPACK), y) > > We don't usually add a space after the comma in an ifeq, especially in > cases like this simple test. > > Why duplicate the dependency on lapack/clapack in the !openblas case, > when the same dependencies already exist, below? > > I mean: in the !openblas case, we know that either lapack or clapack are > enabled, so we know will hit either case in the block [...] > >> +ARMADILLO_DEPENDENCIES = clapack >> +else >> +ARMADILLO_DEPENDENCIES = lapack >> +endif >> +endif > > [...] here: > >> +# lapack support may be provided by lapack or clapack >> +# but not by openblas >> +ifeq ($(BR2_PACKAGE_CLAPACK),y) >> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON >> +ARMADILLO_DEPENDENCIES += clapack >> +else ifeq ($(BR2_PACKAGE_LAPACK),y) >> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON >> +ARMADILLO_DEPENDENCIES += lapack >> +else >> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=OFF >> +endif > > Additionally, this block will be hit even in the openblas case. Is this > expected? Historical accident. This is reworked from v3 which had an explicit Config.in option for the lapack choice I'll send a v5 which fixes it. I still want Gwenhael to confirm that it's good in this shape though. Regards, Arnout
On 25/07/2021 11:36, Arnout Vandecappelle wrote: > I'll send a v5 which fixes it. I still want Gwenhael to confirm that it's good > in this shape though. There won't be a v5 coming because I can't overcome the circular dependency. I think, actually, that we should just drop the clapack package. It's dead (the lapack maintainers no longer generate clapack based on new lapack releases, probably because f2c is unmaintained). I remember Romain and I were banging our heads against lapack, clapack and openblas a couple of years ago on a Buildroot developer meeting - I believe it was in the context of numpy at the time. I don't remember exactly what the conclusion was, but IIRC there was no good solution at the time either. Well, in the end I went ahead and sent a v5 which removes clapack and re-introduces the choice for BLAS provider. The "choice" for LAPACK provider is automatic though (it's just yes or no anyway). Gwenhael, what do you think? Regards, Arnout
diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in index b2b61a3233..710da0314f 100644 --- a/package/armadillo/Config.in +++ b/package/armadillo/Config.in @@ -1,20 +1,34 @@ +config BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS + bool + default y if BR2_PACKAGE_CLAPACK_ARCH_SUPPORTS + default y if BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN + default y if BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS + comment "armadillo needs a toolchain w/ C++" + depends on BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS depends on !BR2_INSTALL_LIBSTDCPP - depends on !BR2_powerpc - depends on !BR2_m68k_cf - -comment "armadillo needs a glibc toolchain w/ C++" - depends on BR2_powerpc - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC config BR2_PACKAGE_ARMADILLO bool "armadillo" + depends on BR2_PACKAGE_ARMADILLO_LAPACK_BLAS_SUPPORTS depends on BR2_INSTALL_LIBSTDCPP - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack - depends on !BR2_m68k_cf # clapack - select BR2_PACKAGE_CLAPACK + select BR2_PACKAGE_CLAPACK if !BR2_PACKAGE_LAPACK && !BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS help Armadillo: An Open Source C++ Linear Algebra Library for Fast Prototyping and Computationally Intensive Experiments. http://arma.sourceforge.net/ + +if BR2_PACKAGE_ARMADILLO + +config BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS + bool "use openblas" + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS + select BR2_PACKAGE_OPENBLAS + help + Use OpenBLAS as BLAS library. Without this option, clapack or lapack + will be used. + +endchoice + +endif diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk index 624b842ef6..82df7602be 100644 --- a/package/armadillo/armadillo.mk +++ b/package/armadillo/armadillo.mk @@ -7,11 +7,37 @@ ARMADILLO_VERSION = 9.900.2 ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma -ARMADILLO_DEPENDENCIES = clapack ARMADILLO_INSTALL_STAGING = YES ARMADILLO_LICENSE = Apache-2.0 ARMADILLO_LICENSE_FILES = LICENSE.txt ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false +# blas support may be provided by lapack, clapack or openblas +# blas library from (c)lapack is libblas.a, libopenblas.a otherwise +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas +ARMADILLO_DEPENDENCIES = openblas +else +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas +ifeq ($(BR2_PACKAGE_CLAPACK), y) +ARMADILLO_DEPENDENCIES = clapack +else +ARMADILLO_DEPENDENCIES = lapack +endif +endif + +# lapack support may be provided by lapack or clapack +# but not by openblas +ifeq ($(BR2_PACKAGE_CLAPACK),y) +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON +ARMADILLO_DEPENDENCIES += clapack +else ifeq ($(BR2_PACKAGE_LAPACK),y) +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON +ARMADILLO_DEPENDENCIES += lapack +else +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=OFF +endif + $(eval $(cmake-package))