[Buildroot] [PATCH v6] openblas: new package

[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera]

Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
---
Changes v5 -> v6:
 - Add BR2_PACKAGE_OPENBLAS_ARCH_SUPPORT
 - Put openblas targets that depend on "Target Architecture" at bottom.
   We want the ones depeding on "Target Architecture Variant" first as
   they are more specific and Buildroot will use the one that matches
   first.
Changes v4 -> v5:
 - Do not use a huge choice with new Buildroot options for every
   OpenBLAS target. Instead, use a string option with a default value
   depending on the existing options in arch/Config.in*.

Changes v3 -> v4:
 - Disable Fortran by default. It can be added back in the future when
   we have a BR2_TOOLCHAIN_HAS_FORTRAN hidden symbol.
 - Bump version so we can get rid of the patch.
 - Match OpenBLAS target cores with Buildroot target cores as much as
   possible, and drop the ones unsupported in Buildroot.
 - Drop the OpenMP bits in the openblas.mk file. It can be added back
   in the future when we have a BR2_TOOLCHAIN_HAS_OPENMP hidden symbol.

Changes v2 -> v3:
 - Switch version to HEAD of develop branch as it contains support for
   MIPS32.
 - Add a patch to fix build failure of P5600 and I6400 targets.
 - Add support for OpenMP when toolchain has NPTL threads.

Changes v1 -> v2:
 - Multi-threading is not available for static-only since it uses
   dlfcn.h, so modify the "ifeq" statement accordingly.

 package/Config.in              |  1 +
 package/openblas/Config.in     | 62 ++++++++++++++++++++++++++++++++++++++++++
 package/openblas/openblas.hash |  2 ++
 package/openblas/openblas.mk   | 46 +++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)
 create mode 100644 package/openblas/Config.in
 create mode 100644 package/openblas/openblas.hash
 create mode 100644 package/openblas/openblas.mk

diff --git a/package/Config.in b/package/Config.in
index 02cc3bd..e7aa6fd 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -1291,6 +1291,7 @@ endif
 	source "package/msgpack/Config.in"
 	source "package/mtdev2tuio/Config.in"
 	source "package/netbsd-queue/Config.in"
+	source "package/openblas/Config.in"
 	source "package/orc/Config.in"
 	source "package/p11-kit/Config.in"
 	source "package/poco/Config.in"
diff --git a/package/openblas/Config.in b/package/openblas/Config.in
new file mode 100644
index 0000000..08389e2
--- /dev/null
+++ b/package/openblas/Config.in
@@ -0,0 +1,62 @@
+config BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
+	bool
+	default y if BR2_i386 || BR2_x86_64
+	default y if BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
+	default y if BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el
+	default y if BR2_sparc || BR2_sparc64
+	default y if BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be
+
+config BR2_PACKAGE_OPENBLAS
+	bool "openblas"
+	depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
+	help
+	  An optimized BLAS library based on GotoBLAS2 1.13 BSD version.
+
+	  https://www.openblas.net/
+
+if BR2_PACKAGE_OPENBLAS
+
+config BR2_PACKAGE_OPENBLAS_TARGET
+	string "OpenBLAS target CPU"
+	# "Target Architecture Variant" matches
+	default "P2"           if BR2_x86_pentium2
+	default "KATMAI"       if BR2_x86_pentium3
+	default "NORTHWOOD"    if BR2_x86_pentium4
+	default "PRESCOTT"     if BR2_x86_prescott
+	default "BANIAS"       if BR2_x86_pentium_m
+	default "CORE2"        if BR2_x86_core2
+	default "NEHALEM"      if BR2_x86_corei7
+	default "SANDYBRIDGE"  if BR2_x86_corei7_avx
+	default "HASWELL"      if BR2_x86_core_avx2
+	default "ATOM"         if BR2_x86_atom
+	default "ATHLON"       if BR2_x86_athlon || BR2_x86_athlon_4
+	default "OPTERON"      if BR2_x86_opteron
+	default "OPTERON_SSE3" if BR2_x86_opteron_sse3
+	default "BARCELONA"    if BR2_x86_barcelona
+	default "STEAMROLLER"  if BR2_x86_steamroller
+	default "VIAC3"        if BR2_x86_c3 || BR2_x86_c32
+	default "POWER4"       if BR2_powerpc_power4
+	default "POWER5"       if BR2_powerpc_power5
+	default "POWER6"       if BR2_powerpc_power6
+	default "POWER7"       if BR2_powerpc_power7
+	default "POWER8"       if BR2_powerpc_power8
+	default "PPCG4"        if BR2_powerpc_7400 || BR2_powerpc_7450
+	default "PPC970"       if BR2_powerpc_970
+	default "PPC440"       if BR2_powerpc_440
+	default "PPC440FP2"    if BR2_powerpc_440fp
+	default "P5600"        if BR2_mips_32r2
+	default "SICORTEX"     if BR2_mips_64
+	default "I6400"        if BR2_mips_64r6
+	default "CORTEXA15"    if BR2_cortex_a15
+	default "CORTEXA9"     if BR2_cortex_a9
+	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
+	                          BR2_cortex_a8 || BR2_cortex_a9 || \
+				  BR2_cortex_a12 || BR2_cortex_a17
+	# "Target Architecture" matches
+	default "SSE_GENERIC"  if BR2_i386 || BR2_x86_64
+	default "SPARC"        if BR2_sparc
+	default "ARMV8"        if BR2_aarch64 || BR2_aarch64_be
+	help
+	  OpenBLAS target CPU
+
+endif
diff --git a/package/openblas/openblas.hash b/package/openblas/openblas.hash
new file mode 100644
index 0000000..1fdb0ba
--- /dev/null
+++ b/package/openblas/openblas.hash
@@ -0,0 +1,2 @@
+# Locally calculated
+sha256 fa32d00dfca6b7e7580dbc8696daa5bf8fee4ad7771f52450ab9dc1e9c87fe73  openblas-a8fcd89d6d1666185c8c27ea46672b9897630f21.tar.gz
diff --git a/package/openblas/openblas.mk b/package/openblas/openblas.mk
new file mode 100644
index 0000000..02dc361
--- /dev/null
+++ b/package/openblas/openblas.mk
@@ -0,0 +1,46 @@
+################################################################################
+#
+# openblas
+#
+################################################################################
+
+OPENBLAS_VERSION = a8fcd89d6d1666185c8c27ea46672b9897630f21
+OPENBLAS_SITE = $(call github,xianyi,OpenBLAS,$(OPENBLAS_VERSION))
+OPENBLAS_LICENSE = BSD-3
+OPENBLAS_LICENSE_FILES = LICENSE
+OPENBLAS_INSTALL_STAGING = YES
+
+# Disable fortran by default until we add BR2_TOOLCHAIN_HAS_FORTRAN
+# hidden symbol to our toolchain infrastructure
+OPENBLAS_MAKE_OPTS += ONLY_CBLAS=1
+
+# Enable/Disable multi-threading (not for static-only since it uses dlfcn.h)
+ifeq ($(BR2_TOOLCHAIN_HAS_THREADS)x$(BR2_STATIC_LIBS),yx)
+OPENBLAS_MAKE_OPTS += USE_THREAD=1
+else
+OPENBLAS_MAKE_OPTS += USE_THREAD=0
+endif
+
+# Static-only/Shared-only toggle
+ifeq ($(BR2_STATIC_LIBS),y)
+OPENBLAS_MAKE_OPTS += NO_SHARED=1
+else ifeq ($(BR2_SHARED_LIBS),y)
+OPENBLAS_MAKE_OPTS += NO_STATIC=1
+endif
+
+define OPENBLAS_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) $(OPENBLAS_MAKE_OPTS) \
+		CROSS=1 TARGET=$(BR2_PACKAGE_OPENBLAS_TARGET) -C $(@D)
+endef
+
+define OPENBLAS_INSTALL_STAGING_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
+		-C $(@D) install PREFIX=$(STAGING_DIR)/usr
+endef
+
+define OPENBLAS_INSTALL_TARGET_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
+		-C $(@D) install PREFIX=$(TARGET_DIR)/usr
+endef
+
+$(eval $(generic-package))
-- 
2.7.3

[Buildroot] [PATCH v6] openblas: new package

[Baruch Siach]

Hi Vicente,

On Mon, Jun 20, 2016 at 05:45:42PM +0100, Vicente Olivert Riera wrote:
> +OPENBLAS_VERSION = a8fcd89d6d1666185c8c27ea46672b9897630f21
> +OPENBLAS_SITE = $(call github,xianyi,OpenBLAS,$(OPENBLAS_VERSION))
> +OPENBLAS_LICENSE = BSD-3

BSD-3c, isn't it?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[Buildroot] [PATCH v6] openblas: new package

[Yann E. MORIN]

Vincent, All,

[Gah, you were too fast respinning after our IRC discussion, I said I
did not yet do a complete review and that I was going to do one "soon".
Here it is! ;-) ]

On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
> Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
> ---
[--SNIP--]
> diff --git a/package/openblas/Config.in b/package/openblas/Config.in
> new file mode 100644
> index 0000000..08389e2
> --- /dev/null
> +++ b/package/openblas/Config.in
> @@ -0,0 +1,62 @@
> +config BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
> +	bool
> +	default y if BR2_i386 || BR2_x86_64
> +	default y if BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
> +	default y if BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el
> +	default y if BR2_sparc || BR2_sparc64
> +	default y if BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be
> +
> +config BR2_PACKAGE_OPENBLAS
> +	bool "openblas"
> +	depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
> +	help
> +	  An optimized BLAS library based on GotoBLAS2 1.13 BSD version.
> +
> +	  https://www.openblas.net/
> +
> +if BR2_PACKAGE_OPENBLAS
> +
> +config BR2_PACKAGE_OPENBLAS_TARGET
> +	string "OpenBLAS target CPU"
> +	# "Target Architecture Variant" matches
> +	default "P2"           if BR2_x86_pentium2
> +	default "KATMAI"       if BR2_x86_pentium3
> +	default "NORTHWOOD"    if BR2_x86_pentium4
> +	default "PRESCOTT"     if BR2_x86_prescott
> +	default "BANIAS"       if BR2_x86_pentium_m
> +	default "CORE2"        if BR2_x86_core2
> +	default "NEHALEM"      if BR2_x86_corei7
> +	default "SANDYBRIDGE"  if BR2_x86_corei7_avx
> +	default "HASWELL"      if BR2_x86_core_avx2
> +	default "ATOM"         if BR2_x86_atom
> +	default "ATHLON"       if BR2_x86_athlon || BR2_x86_athlon_4
> +	default "OPTERON"      if BR2_x86_opteron
> +	default "OPTERON_SSE3" if BR2_x86_opteron_sse3
> +	default "BARCELONA"    if BR2_x86_barcelona
> +	default "STEAMROLLER"  if BR2_x86_steamroller
> +	default "VIAC3"        if BR2_x86_c3 || BR2_x86_c32
> +	default "POWER4"       if BR2_powerpc_power4
> +	default "POWER5"       if BR2_powerpc_power5
> +	default "POWER6"       if BR2_powerpc_power6
> +	default "POWER7"       if BR2_powerpc_power7
> +	default "POWER8"       if BR2_powerpc_power8
> +	default "PPCG4"        if BR2_powerpc_7400 || BR2_powerpc_7450
> +	default "PPC970"       if BR2_powerpc_970
> +	default "PPC440"       if BR2_powerpc_440
> +	default "PPC440FP2"    if BR2_powerpc_440fp
> +	default "P5600"        if BR2_mips_32r2
> +	default "SICORTEX"     if BR2_mips_64
> +	default "I6400"        if BR2_mips_64r6
> +	default "CORTEXA15"    if BR2_cortex_a15
> +	default "CORTEXA9"     if BR2_cortex_a9
> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
> +				  BR2_cortex_a12 || BR2_cortex_a17

Incorrect indentation for the ARM variants.

Also, cortex_a9 is duplicated for armv7: it already has its own entry.

> +	# "Target Architecture" matches
> +	default "SSE_GENERIC"  if BR2_i386 || BR2_x86_64

Are you sure we want to default to "SSE_GENERIC" for i386? AFAIK, SSE
arrived quite late in the x86 32-bit line; CPUs up to and including some
of the pentiums do not have SSE.

For example i486, i586, x1000, i686, pentiumpro, pentium_mmx, pentium2,
k6, k6_2 and athlon do not have SSE.

Is there an even lower "target CPU" that openBLAS knows of? Or should we
just disable openBLAS for the aforementioned CPUs?

Note: I haven't looked at the other archs, but arm springs to mind too
(what would be the value for armv4, armv5 or armv6? Should it also be
disabled for those as well?)

> +	default "SPARC"        if BR2_sparc
> +	default "ARMV8"        if BR2_aarch64 || BR2_aarch64_be
> +	help
> +	  OpenBLAS target CPU
> +
> +endif
> diff --git a/package/openblas/openblas.hash b/package/openblas/openblas.hash
> new file mode 100644
> index 0000000..1fdb0ba
> --- /dev/null
> +++ b/package/openblas/openblas.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256 fa32d00dfca6b7e7580dbc8696daa5bf8fee4ad7771f52450ab9dc1e9c87fe73  openblas-a8fcd89d6d1666185c8c27ea46672b9897630f21.tar.gz
> diff --git a/package/openblas/openblas.mk b/package/openblas/openblas.mk
> new file mode 100644
> index 0000000..02dc361
> --- /dev/null
> +++ b/package/openblas/openblas.mk
> @@ -0,0 +1,46 @@
> +################################################################################
> +#
> +# openblas
> +#
> +################################################################################
> +
> +OPENBLAS_VERSION = a8fcd89d6d1666185c8c27ea46672b9897630f21

Why can't we use a release (quite recent, 0.2.18 was relased in April)?

> +OPENBLAS_SITE = $(call github,xianyi,OpenBLAS,$(OPENBLAS_VERSION))
> +OPENBLAS_LICENSE = BSD-3
> +OPENBLAS_LICENSE_FILES = LICENSE
> +OPENBLAS_INSTALL_STAGING = YES
> +
> +# Disable fortran by default until we add BR2_TOOLCHAIN_HAS_FORTRAN
> +# hidden symbol to our toolchain infrastructure
> +OPENBLAS_MAKE_OPTS += ONLY_CBLAS=1

No need for += here, it's the first assignment.

> +# Enable/Disable multi-threading (not for static-only since it uses dlfcn.h)
> +ifeq ($(BR2_TOOLCHAIN_HAS_THREADS)x$(BR2_STATIC_LIBS),yx)

Nit-pick: I'd prefer we use ':' as a separator.

(We currently have both: 6 use 'x', 12 use ':', so I stand that we
should use ':' ;-] But I won;t block just for that. ;-) )

> +OPENBLAS_MAKE_OPTS += USE_THREAD=1
> +else
> +OPENBLAS_MAKE_OPTS += USE_THREAD=0
> +endif
> +
> +# Static-only/Shared-only toggle
> +ifeq ($(BR2_STATIC_LIBS),y)
> +OPENBLAS_MAKE_OPTS += NO_SHARED=1
> +else ifeq ($(BR2_SHARED_LIBS),y)
> +OPENBLAS_MAKE_OPTS += NO_STATIC=1
> +endif

What about BR2_SHARED_STATIC_LIBS ?

I guess in this case, you want to behave as for BR2_SHARED_LIBS (i.e.
only build shared libs).

> +define OPENBLAS_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) $(OPENBLAS_MAKE_OPTS) \
> +		CROSS=1 TARGET=$(BR2_PACKAGE_OPENBLAS_TARGET) -C $(@D)

Move CROSS=1 and TARGET=$(BR2_PACKAGE_OPENBLAS_TARGET) in OPENBLAS_MAKE_OPTS,
so that they also get passed during install (for consistency).

> +endef
> +
> +define OPENBLAS_INSTALL_STAGING_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
> +		-C $(@D) install PREFIX=$(STAGING_DIR)/usr
> +endef

On IRC, you said that it was "very unlikely" that there would be a
package that depends on OpenBLAS in the future. So, what is the point in
installing it to staging if no package will ever use it?

If you don;t plan on adding such a package in the future, no need to
install into staging; we can very well add it at the time we add our
first package that needs OpenBLAS.

Or do you have a hidden, unmentionable reason? ;-]

> +define OPENBLAS_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
> +		-C $(@D) install PREFIX=$(TARGET_DIR)/usr
> +endef
> +
> +$(eval $(generic-package))

Well, they have a CMakelist.txt; why can't we use cmake-package?

> -- 
> 2.7.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera]

Hello Yann, thanks you very much for your review. Below are some
comments, please keep scrolling.

On 20/06/16 18:27, Yann E. MORIN wrote:
> Vincent, All,
> 
> [Gah, you were too fast respinning after our IRC discussion, I said I
> did not yet do a complete review and that I was going to do one "soon".
> Here it is! ;-) ]
> 
> On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
>> Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
>> ---
> [--SNIP--]
>> diff --git a/package/openblas/Config.in b/package/openblas/Config.in
>> new file mode 100644
>> index 0000000..08389e2
>> --- /dev/null
>> +++ b/package/openblas/Config.in
>> @@ -0,0 +1,62 @@
>> +config BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
>> +	bool
>> +	default y if BR2_i386 || BR2_x86_64
>> +	default y if BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
>> +	default y if BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el
>> +	default y if BR2_sparc || BR2_sparc64
>> +	default y if BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be
>> +
>> +config BR2_PACKAGE_OPENBLAS
>> +	bool "openblas"
>> +	depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
>> +	help
>> +	  An optimized BLAS library based on GotoBLAS2 1.13 BSD version.
>> +
>> +	  https://www.openblas.net/
>> +
>> +if BR2_PACKAGE_OPENBLAS
>> +
>> +config BR2_PACKAGE_OPENBLAS_TARGET
>> +	string "OpenBLAS target CPU"
>> +	# "Target Architecture Variant" matches
>> +	default "P2"           if BR2_x86_pentium2
>> +	default "KATMAI"       if BR2_x86_pentium3
>> +	default "NORTHWOOD"    if BR2_x86_pentium4
>> +	default "PRESCOTT"     if BR2_x86_prescott
>> +	default "BANIAS"       if BR2_x86_pentium_m
>> +	default "CORE2"        if BR2_x86_core2
>> +	default "NEHALEM"      if BR2_x86_corei7
>> +	default "SANDYBRIDGE"  if BR2_x86_corei7_avx
>> +	default "HASWELL"      if BR2_x86_core_avx2
>> +	default "ATOM"         if BR2_x86_atom
>> +	default "ATHLON"       if BR2_x86_athlon || BR2_x86_athlon_4
>> +	default "OPTERON"      if BR2_x86_opteron
>> +	default "OPTERON_SSE3" if BR2_x86_opteron_sse3
>> +	default "BARCELONA"    if BR2_x86_barcelona
>> +	default "STEAMROLLER"  if BR2_x86_steamroller
>> +	default "VIAC3"        if BR2_x86_c3 || BR2_x86_c32
>> +	default "POWER4"       if BR2_powerpc_power4
>> +	default "POWER5"       if BR2_powerpc_power5
>> +	default "POWER6"       if BR2_powerpc_power6
>> +	default "POWER7"       if BR2_powerpc_power7
>> +	default "POWER8"       if BR2_powerpc_power8
>> +	default "PPCG4"        if BR2_powerpc_7400 || BR2_powerpc_7450
>> +	default "PPC970"       if BR2_powerpc_970
>> +	default "PPC440"       if BR2_powerpc_440
>> +	default "PPC440FP2"    if BR2_powerpc_440fp
>> +	default "P5600"        if BR2_mips_32r2
>> +	default "SICORTEX"     if BR2_mips_64
>> +	default "I6400"        if BR2_mips_64r6
>> +	default "CORTEXA15"    if BR2_cortex_a15
>> +	default "CORTEXA9"     if BR2_cortex_a9
>> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
>> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
>> +				  BR2_cortex_a12 || BR2_cortex_a17
> 
> Incorrect indentation for the ARM variants.

Fixed.

> Also, cortex_a9 is duplicated for armv7: it already has its own entry.

Fixed.

> 
>> +	# "Target Architecture" matches
>> +	default "SSE_GENERIC"  if BR2_i386 || BR2_x86_64
> 
> Are you sure we want to default to "SSE_GENERIC" for i386? AFAIK, SSE
> arrived quite late in the x86 32-bit line; CPUs up to and including some
> of the pentiums do not have SSE.

Ok, SSE_GENERIC only for BR2_x86_64.

> For example i486, i586, x1000, i686, pentiumpro, pentium_mmx, pentium2,
> k6, k6_2 and athlon do not have SSE.
> 
> Is there an even lower "target CPU" that openBLAS knows of? Or should we
> just disable openBLAS for the aforementioned CPUs?
> 
> Note: I haven't looked at the other archs, but arm springs to mind too
> (what would be the value for armv4, armv5 or armv6? Should it also be
> disabled for those as well?)

So you want me to disable this package for every core we have in
Buildroot that doesn't have a matching OpenBLAS core?

> 
>> +	default "SPARC"        if BR2_sparc
>> +	default "ARMV8"        if BR2_aarch64 || BR2_aarch64_be
>> +	help
>> +	  OpenBLAS target CPU
>> +
>> +endif
>> diff --git a/package/openblas/openblas.hash b/package/openblas/openblas.hash
>> new file mode 100644
>> index 0000000..1fdb0ba
>> --- /dev/null
>> +++ b/package/openblas/openblas.hash
>> @@ -0,0 +1,2 @@
>> +# Locally calculated
>> +sha256 fa32d00dfca6b7e7580dbc8696daa5bf8fee4ad7771f52450ab9dc1e9c87fe73  openblas-a8fcd89d6d1666185c8c27ea46672b9897630f21.tar.gz
>> diff --git a/package/openblas/openblas.mk b/package/openblas/openblas.mk
>> new file mode 100644
>> index 0000000..02dc361
>> --- /dev/null
>> +++ b/package/openblas/openblas.mk
>> @@ -0,0 +1,46 @@
>> +################################################################################
>> +#
>> +# openblas
>> +#
>> +################################################################################
>> +
>> +OPENBLAS_VERSION = a8fcd89d6d1666185c8c27ea46672b9897630f21
> 
> Why can't we use a release (quite recent, 0.2.18 was relased in April)?

Because there have been important changes for MIPS after that release.
In fact in the next respin I will use a more recent commit.

>> +OPENBLAS_SITE = $(call github,xianyi,OpenBLAS,$(OPENBLAS_VERSION))
>> +OPENBLAS_LICENSE = BSD-3

I'll put BSD-3c here, thanks Baruch.

>> +OPENBLAS_LICENSE_FILES = LICENSE
>> +OPENBLAS_INSTALL_STAGING = YES
>> +
>> +# Disable fortran by default until we add BR2_TOOLCHAIN_HAS_FORTRAN
>> +# hidden symbol to our toolchain infrastructure
>> +OPENBLAS_MAKE_OPTS += ONLY_CBLAS=1
> 
> No need for += here, it's the first assignment.

No += for the fist assignment anymore.

>> +# Enable/Disable multi-threading (not for static-only since it uses dlfcn.h)
>> +ifeq ($(BR2_TOOLCHAIN_HAS_THREADS)x$(BR2_STATIC_LIBS),yx)
> 
> Nit-pick: I'd prefer we use ':' as a separator.
> 
> (We currently have both: 6 use 'x', 12 use ':', so I stand that we
> should use ':' ;-] But I won;t block just for that. ;-) )

Done.

>> +OPENBLAS_MAKE_OPTS += USE_THREAD=1
>> +else
>> +OPENBLAS_MAKE_OPTS += USE_THREAD=0
>> +endif
>> +
>> +# Static-only/Shared-only toggle
>> +ifeq ($(BR2_STATIC_LIBS),y)
>> +OPENBLAS_MAKE_OPTS += NO_SHARED=1
>> +else ifeq ($(BR2_SHARED_LIBS),y)
>> +OPENBLAS_MAKE_OPTS += NO_STATIC=1
>> +endif
> 
> What about BR2_SHARED_STATIC_LIBS ?
> 
> I guess in this case, you want to behave as for BR2_SHARED_LIBS (i.e.
> only build shared libs).

No, why? For BR2_SHARED_STATIC_LIBS I just want the default behavior
which will build both shared and static libraries.

>> +define OPENBLAS_BUILD_CMDS
>> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) $(OPENBLAS_MAKE_OPTS) \
>> +		CROSS=1 TARGET=$(BR2_PACKAGE_OPENBLAS_TARGET) -C $(@D)
> 
> Move CROSS=1 and TARGET=$(BR2_PACKAGE_OPENBLAS_TARGET) in OPENBLAS_MAKE_OPTS,
> so that they also get passed during install (for consistency).

Done.

>> +endef
>> +
>> +define OPENBLAS_INSTALL_STAGING_CMDS
>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>> +		-C $(@D) install PREFIX=$(STAGING_DIR)/usr
>> +endef
> 
> On IRC, you said that it was "very unlikely" that there would be a
> package that depends on OpenBLAS in the future. So, what is the point in
> installing it to staging if no package will ever use it?

Forget about that, I didn't say anything :-)

> If you don;t plan on adding such a package in the future, no need to
> install into staging; we can very well add it at the time we add our
> first package that needs OpenBLAS.
> 
> Or do you have a hidden, unmentionable reason? ;-]

Well, it's a library so there may be other packages who will use it in
order to be built at some point. Is not the default policy to install
library packages to staging? I thought it was, but maybe I'm wrong.

Would it be so bad to install it to staging just in case someone wants
to develop a package which depends on OpenBLAS but for some reason that
package cannot be added to Buildroot upstream? That way the user will
not need to modify the OpenBLAS package in order to add the
install_staging line plus the install_target_cmds function.

>> +define OPENBLAS_INSTALL_TARGET_CMDS
>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>> +		-C $(@D) install PREFIX=$(TARGET_DIR)/usr
>> +endef
>> +
>> +$(eval $(generic-package))
> 
> Well, they have a CMakelist.txt; why can't we use cmake-package?

For two reasons. The first one is that the official documentation uses
make in the installation guide.

The second one is this line in the CMakelist.txt file:

message(WARNING "CMake support is experimental. This will not produce
the same Makefiles that OpenBLAS ships with. Only x86 support is
currently available.")

Regards,

Vincent.

> 
>> -- 
>> 2.7.3
>>
> 

[Buildroot] [PATCH v6] openblas: new package

[Thomas Petazzoni]

Hello,

On Mon, 20 Jun 2016 17:45:42 +0100, Vicente Olivert Riera wrote:

> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
> +				  BR2_cortex_a12 || BR2_cortex_a17

You can use BR2_ARM_CPU_ARMV7A instead here.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera]

Hello Thomas,
On 21/06/16 12:51, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 20 Jun 2016 17:45:42 +0100, Vicente Olivert Riera wrote:
> 
>> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
>> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
>> +				  BR2_cortex_a12 || BR2_cortex_a17
> 
> You can use BR2_ARM_CPU_ARMV7A instead here.

I'll do it, thanks!

Regards,

Vincent.

> 
> Thomas
> 

[Buildroot] [PATCH v6] openblas: new package

[Arnout Vandecappelle]

On 21-06-16 11:50, Vicente Olivert Riera wrote:
> Hello Yann, thanks you very much for your review. Below are some
> comments, please keep scrolling.
> 
> On 20/06/16 18:27, Yann E. MORIN wrote:
>> Vincent, All,
>>
>> [Gah, you were too fast respinning after our IRC discussion, I said I
>> did not yet do a complete review and that I was going to do one "soon".
>> Here it is! ;-) ]
>>
>> On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
[snip]
>>> +config BR2_PACKAGE_OPENBLAS_TARGET
>>> +	string "OpenBLAS target CPU"

 This smells fishy to me. Why do we ask the user to fill this in? I mean, if
we're building for power4, does it make sense to set this to e.g. CORE2?

 In other words, I think this should be a blind option.

 Which raises the next question: what happens for CPUs not in the list? What
happens when we leave it empty?

 If OpenBLAS is simply not supported on CPUs not in this list, then I think we
should use this list as the architecture dependency of the package. I.e., make
it a blind option, and add to the main symbol:

	depends on BR2_PACKAGE_OPENBLAS_TARGET != ""

>>> +	# "Target Architecture Variant" matches
>>> +	default "P2"           if BR2_x86_pentium2
>>> +	default "KATMAI"       if BR2_x86_pentium3
>>> +	default "NORTHWOOD"    if BR2_x86_pentium4
>>> +	default "PRESCOTT"     if BR2_x86_prescott
>>> +	default "BANIAS"       if BR2_x86_pentium_m
>>> +	default "CORE2"        if BR2_x86_core2
>>> +	default "NEHALEM"      if BR2_x86_corei7
>>> +	default "SANDYBRIDGE"  if BR2_x86_corei7_avx
>>> +	default "HASWELL"      if BR2_x86_core_avx2
>>> +	default "ATOM"         if BR2_x86_atom
>>> +	default "ATHLON"       if BR2_x86_athlon || BR2_x86_athlon_4
>>> +	default "OPTERON"      if BR2_x86_opteron
>>> +	default "OPTERON_SSE3" if BR2_x86_opteron_sse3
>>> +	default "BARCELONA"    if BR2_x86_barcelona
>>> +	default "STEAMROLLER"  if BR2_x86_steamroller
>>> +	default "VIAC3"        if BR2_x86_c3 || BR2_x86_c32
>>> +	default "POWER4"       if BR2_powerpc_power4
>>> +	default "POWER5"       if BR2_powerpc_power5
>>> +	default "POWER6"       if BR2_powerpc_power6
>>> +	default "POWER7"       if BR2_powerpc_power7
>>> +	default "POWER8"       if BR2_powerpc_power8
>>> +	default "PPCG4"        if BR2_powerpc_7400 || BR2_powerpc_7450
>>> +	default "PPC970"       if BR2_powerpc_970
>>> +	default "PPC440"       if BR2_powerpc_440
>>> +	default "PPC440FP2"    if BR2_powerpc_440fp
>>> +	default "P5600"        if BR2_mips_32r2
>>> +	default "SICORTEX"     if BR2_mips_64
>>> +	default "I6400"        if BR2_mips_64r6
>>> +	default "CORTEXA15"    if BR2_cortex_a15
>>> +	default "CORTEXA9"     if BR2_cortex_a9
>>> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
>>> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
>>> +				  BR2_cortex_a12 || BR2_cortex_a17
>>
>> Incorrect indentation for the ARM variants.
> 
> Fixed.
> 
>> Also, cortex_a9 is duplicated for armv7: it already has its own entry.
> 
> Fixed.
> 
>>
>>> +	# "Target Architecture" matches

 I would keep these together with their corresponding CPU options above. So
first all the specific x86 CPUs, and then the SSE_GENERIC one; then the
powerpcs, etc.

>>> +	default "SSE_GENERIC"  if BR2_i386 || BR2_x86_64
>>
>> Are you sure we want to default to "SSE_GENERIC" for i386? AFAIK, SSE
>> arrived quite late in the x86 32-bit line; CPUs up to and including some
>> of the pentiums do not have SSE.
> 
> Ok, SSE_GENERIC only for BR2_x86_64.

 I would say: use BR2_X86_CPU_HAS_SSE

> 
>> For example i486, i586, x1000, i686, pentiumpro, pentium_mmx, pentium2,
>> k6, k6_2 and athlon do not have SSE.
>>
>> Is there an even lower "target CPU" that openBLAS knows of? Or should we
>> just disable openBLAS for the aforementioned CPUs?
>>
>> Note: I haven't looked at the other archs, but arm springs to mind too
>> (what would be the value for armv4, armv5 or armv6? Should it also be
>> disabled for those as well?)
> 
> So you want me to disable this package for every core we have in
> Buildroot that doesn't have a matching OpenBLAS core?

 That depends on whether OpenBLAS works on them or not. This is absolutely not
clear from this patch. Please explain this in the commit log or in comments in
your next version.

> 
>>
>>> +	default "SPARC"        if BR2_sparc
>>> +	default "ARMV8"        if BR2_aarch64 || BR2_aarch64_be
>>> +	help
>>> +	  OpenBLAS target CPU
>>> +
>>> +endif
[snip]
>>> +define OPENBLAS_INSTALL_STAGING_CMDS
>>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>>> +		-C $(@D) install PREFIX=$(STAGING_DIR)/usr
>>> +endef
>>
>> On IRC, you said that it was "very unlikely" that there would be a
>> package that depends on OpenBLAS in the future. So, what is the point in
>> installing it to staging if no package will ever use it?
> 
> Forget about that, I didn't say anything :-)
> 
>> If you don;t plan on adding such a package in the future, no need to
>> install into staging; we can very well add it at the time we add our
>> first package that needs OpenBLAS.
>>
>> Or do you have a hidden, unmentionable reason? ;-]
> 
> Well, it's a library so there may be other packages who will use it in
> order to be built at some point. Is not the default policy to install
> library packages to staging? I thought it was, but maybe I'm wrong.

 Of course it is, Yann was talking gibberish. We have plenty of libraries on
which no package depends. For example, most of the qt5 sublibraries.


 Regards,
 Arnout


> 
> Would it be so bad to install it to staging just in case someone wants
> to develop a package which depends on OpenBLAS but for some reason that
> package cannot be added to Buildroot upstream? That way the user will
> not need to modify the OpenBLAS package in order to add the
> install_staging line plus the install_target_cmds function.
> 
>>> +define OPENBLAS_INSTALL_TARGET_CMDS
>>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>>> +		-C $(@D) install PREFIX=$(TARGET_DIR)/usr
>>> +endef
>>> +
>>> +$(eval $(generic-package))
>>
>> Well, they have a CMakelist.txt; why can't we use cmake-package?
> 
> For two reasons. The first one is that the official documentation uses
> make in the installation guide.
> 
> The second one is this line in the CMakelist.txt file:
> 
> message(WARNING "CMake support is experimental. This will not produce
> the same Makefiles that OpenBLAS ships with. Only x86 support is
> currently available.")
> 
> Regards,
> 
> Vincent.
> 
>>
>>> -- 
>>> 2.7.3
>>>
>>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera]

Hello Arnout, thanks a lot for your review.

Below are some comments, please keep reading.

On 22/06/16 22:17, Arnout Vandecappelle wrote:
> On 21-06-16 11:50, Vicente Olivert Riera wrote:
>> Hello Yann, thanks you very much for your review. Below are some
>> comments, please keep scrolling.
>>
>> On 20/06/16 18:27, Yann E. MORIN wrote:
>>> Vincent, All,
>>>
>>> [Gah, you were too fast respinning after our IRC discussion, I said I
>>> did not yet do a complete review and that I was going to do one "soon".
>>> Here it is! ;-) ]
>>>
>>> On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
> [snip]
>>>> +config BR2_PACKAGE_OPENBLAS_TARGET
>>>> +	string "OpenBLAS target CPU"
> 
>  This smells fishy to me. Why do we ask the user to fill this in? I mean, if
> we're building for power4, does it make sense to set this to e.g. CORE2?
> 
>  In other words, I think this should be a blind option.

there are OpenBLAS targets that apply for the same core. For instance:

- KATMAI and COPPERMINE are pentium3.
- BANIAS and YONAH are pentium_m.
- CORE2, PENRYN and DUNNINGTON are core2.

So as per Thomas suggestion we choose one by default but let the user
change it.

>  Which raises the next question: what happens for CPUs not in the list? What
> happens when we leave it empty?

If you leave it empty it will try to use SANDYBRIDGE and it will fail.

>  If OpenBLAS is simply not supported on CPUs not in this list, then I think we
> should use this list as the architecture dependency of the package. I.e., make
> it a blind option, and add to the main symbol:
> 
> 	depends on BR2_PACKAGE_OPENBLAS_TARGET != ""

As I said above, the same core can have different OpenBLAS cores, so the
blind option doesn't fit.

>>>> +	# "Target Architecture Variant" matches
>>>> +	default "P2"           if BR2_x86_pentium2
>>>> +	default "KATMAI"       if BR2_x86_pentium3
>>>> +	default "NORTHWOOD"    if BR2_x86_pentium4
>>>> +	default "PRESCOTT"     if BR2_x86_prescott
>>>> +	default "BANIAS"       if BR2_x86_pentium_m
>>>> +	default "CORE2"        if BR2_x86_core2
>>>> +	default "NEHALEM"      if BR2_x86_corei7
>>>> +	default "SANDYBRIDGE"  if BR2_x86_corei7_avx
>>>> +	default "HASWELL"      if BR2_x86_core_avx2
>>>> +	default "ATOM"         if BR2_x86_atom
>>>> +	default "ATHLON"       if BR2_x86_athlon || BR2_x86_athlon_4
>>>> +	default "OPTERON"      if BR2_x86_opteron
>>>> +	default "OPTERON_SSE3" if BR2_x86_opteron_sse3
>>>> +	default "BARCELONA"    if BR2_x86_barcelona
>>>> +	default "STEAMROLLER"  if BR2_x86_steamroller
>>>> +	default "VIAC3"        if BR2_x86_c3 || BR2_x86_c32
>>>> +	default "POWER4"       if BR2_powerpc_power4
>>>> +	default "POWER5"       if BR2_powerpc_power5
>>>> +	default "POWER6"       if BR2_powerpc_power6
>>>> +	default "POWER7"       if BR2_powerpc_power7
>>>> +	default "POWER8"       if BR2_powerpc_power8
>>>> +	default "PPCG4"        if BR2_powerpc_7400 || BR2_powerpc_7450
>>>> +	default "PPC970"       if BR2_powerpc_970
>>>> +	default "PPC440"       if BR2_powerpc_440
>>>> +	default "PPC440FP2"    if BR2_powerpc_440fp
>>>> +	default "P5600"        if BR2_mips_32r2
>>>> +	default "SICORTEX"     if BR2_mips_64
>>>> +	default "I6400"        if BR2_mips_64r6
>>>> +	default "CORTEXA15"    if BR2_cortex_a15
>>>> +	default "CORTEXA9"     if BR2_cortex_a9
>>>> +	default "ARMV7"        if BR2_cortex_a5 || BR2_cortex_a7 || \
>>>> +	                          BR2_cortex_a8 || BR2_cortex_a9 || \
>>>> +				  BR2_cortex_a12 || BR2_cortex_a17
>>>
>>> Incorrect indentation for the ARM variants.
>>
>> Fixed.
>>
>>> Also, cortex_a9 is duplicated for armv7: it already has its own entry.
>>
>> Fixed.
>>
>>>
>>>> +	# "Target Architecture" matches
> 
>  I would keep these together with their corresponding CPU options above. So
> first all the specific x86 CPUs, and then the SSE_GENERIC one; then the
> powerpcs, etc.

Ok, no problem.

>>>> +	default "SSE_GENERIC"  if BR2_i386 || BR2_x86_64
>>>
>>> Are you sure we want to default to "SSE_GENERIC" for i386? AFAIK, SSE
>>> arrived quite late in the x86 32-bit line; CPUs up to and including some
>>> of the pentiums do not have SSE.
>>
>> Ok, SSE_GENERIC only for BR2_x86_64.
> 
>  I would say: use BR2_X86_CPU_HAS_SSE

Ok.

>>
>>> For example i486, i586, x1000, i686, pentiumpro, pentium_mmx, pentium2,
>>> k6, k6_2 and athlon do not have SSE.
>>>
>>> Is there an even lower "target CPU" that openBLAS knows of? Or should we
>>> just disable openBLAS for the aforementioned CPUs?
>>>
>>> Note: I haven't looked at the other archs, but arm springs to mind too
>>> (what would be the value for armv4, armv5 or armv6? Should it also be
>>> disabled for those as well?)
>>
>> So you want me to disable this package for every core we have in
>> Buildroot that doesn't have a matching OpenBLAS core?
> 
>  That depends on whether OpenBLAS works on them or not. This is absolutely not
> clear from this patch. Please explain this in the commit log or in comments in
> your next version.

Well, I think is sensible to enable OpenBLAS only for those BR cores who
have an usable OpenBLAS target.

Regards,

Vincent.

> 
>>
>>>
>>>> +	default "SPARC"        if BR2_sparc
>>>> +	default "ARMV8"        if BR2_aarch64 || BR2_aarch64_be
>>>> +	help
>>>> +	  OpenBLAS target CPU
>>>> +
>>>> +endif
> [snip]
>>>> +define OPENBLAS_INSTALL_STAGING_CMDS
>>>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>>>> +		-C $(@D) install PREFIX=$(STAGING_DIR)/usr
>>>> +endef
>>>
>>> On IRC, you said that it was "very unlikely" that there would be a
>>> package that depends on OpenBLAS in the future. So, what is the point in
>>> installing it to staging if no package will ever use it?
>>
>> Forget about that, I didn't say anything :-)
>>
>>> If you don;t plan on adding such a package in the future, no need to
>>> install into staging; we can very well add it at the time we add our
>>> first package that needs OpenBLAS.
>>>
>>> Or do you have a hidden, unmentionable reason? ;-]
>>
>> Well, it's a library so there may be other packages who will use it in
>> order to be built at some point. Is not the default policy to install
>> library packages to staging? I thought it was, but maybe I'm wrong.
> 
>  Of course it is, Yann was talking gibberish. We have plenty of libraries on
> which no package depends. For example, most of the qt5 sublibraries.
> 
> 
>  Regards,
>  Arnout
> 
> 
>>
>> Would it be so bad to install it to staging just in case someone wants
>> to develop a package which depends on OpenBLAS but for some reason that
>> package cannot be added to Buildroot upstream? That way the user will
>> not need to modify the OpenBLAS package in order to add the
>> install_staging line plus the install_target_cmds function.
>>
>>>> +define OPENBLAS_INSTALL_TARGET_CMDS
>>>> +	$(TARGET_MAKE_ENV) $(MAKE) $(OPENBLAS_MAKE_OPTS) \
>>>> +		-C $(@D) install PREFIX=$(TARGET_DIR)/usr
>>>> +endef
>>>> +
>>>> +$(eval $(generic-package))
>>>
>>> Well, they have a CMakelist.txt; why can't we use cmake-package?
>>
>> For two reasons. The first one is that the official documentation uses
>> make in the installation guide.
>>
>> The second one is this line in the CMakelist.txt file:
>>
>> message(WARNING "CMake support is experimental. This will not produce
>> the same Makefiles that OpenBLAS ships with. Only x86 support is
>> currently available.")
>>
>> Regards,
>>
>> Vincent.
>>
>>>
>>>> -- 
>>>> 2.7.3
>>>>
>>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>>
> 
> 

[Buildroot] [PATCH v6] openblas: new package

[Arnout Vandecappelle]

 Hi Vincent,

On 23-06-16 11:30, Vicente Olivert Riera wrote:
> Hello Arnout, thanks a lot for your review.
> 
> Below are some comments, please keep reading.

 I always keep reading :-P

> 
> On 22/06/16 22:17, Arnout Vandecappelle wrote:
>> On 21-06-16 11:50, Vicente Olivert Riera wrote:
>>> Hello Yann, thanks you very much for your review. Below are some
>>> comments, please keep scrolling.
>>>
>>> On 20/06/16 18:27, Yann E. MORIN wrote:
>>>> Vincent, All,
>>>>
>>>> [Gah, you were too fast respinning after our IRC discussion, I said I
>>>> did not yet do a complete review and that I was going to do one "soon".
>>>> Here it is! ;-) ]
>>>>
>>>> On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
>> [snip]
>>>>> +config BR2_PACKAGE_OPENBLAS_TARGET
>>>>> +	string "OpenBLAS target CPU"
>>
>>  This smells fishy to me. Why do we ask the user to fill this in? I mean, if
>> we're building for power4, does it make sense to set this to e.g. CORE2?
>>
>>  In other words, I think this should be a blind option.
> 
> there are OpenBLAS targets that apply for the same core. For instance:
> 
> - KATMAI and COPPERMINE are pentium3.
> - BANIAS and YONAH are pentium_m.
> - CORE2, PENRYN and DUNNINGTON are core2.

 Where do you get this information?

> 
> So as per Thomas suggestion we choose one by default but let the user
> change it.

 OK, I've taken a look at the TargetList.txt. I think actually there are not
many more options than the ones you have listed now, so it _could_ be possible
to add a choice for the missing ones. Alternatively, we could make the option
visible only in the cases where there is ambiguity. However, both options would
complicate things even more than they are now, and we're already at v6, so let's
stick to the current approach. Still:

1. The commit message should clarify this extensively.
2. The help text should explain more clearly what the user should choose.

Commit message:

OpenBLAS is optimised for specific CPU models, which don't fully match with the
GCC code generation options. Therefore, we can't automatically select
BR2_PACKAGE_OPENBLAS_TARGET based on the CPU choice. Instead, let the user
select the TARGET name, but offer a sensible default. Other possible solutions
were deemed too complicated: adding choice options in the ambiguous cases, or
only making the option user-visible when there is ambiguity.


Help text:

	  OpenBLAS target CPU. See TargetList.txt in the source tree for the
	  possible target strings. A possible value is set automatically based
	  on your Target Architecture Variant.

[snip]
>>>> Is there an even lower "target CPU" that openBLAS knows of? Or should we
>>>> just disable openBLAS for the aforementioned CPUs?
>>>>
>>>> Note: I haven't looked at the other archs, but arm springs to mind too
>>>> (what would be the value for armv4, armv5 or armv6? Should it also be
>>>> disabled for those as well?)

 It looks like armv4 is not supported and target should be set to ARMV5 or ARMV6
as appropriate.

>>>
>>> So you want me to disable this package for every core we have in
>>> Buildroot that doesn't have a matching OpenBLAS core?
>>
>>  That depends on whether OpenBLAS works on them or not. This is absolutely not
>> clear from this patch. Please explain this in the commit log or in comments in
>> your next version.
> 
> Well, I think is sensible to enable OpenBLAS only for those BR cores who
> have an usable OpenBLAS target.

 It would indeed be better that way, but again that would complicate things too
much I think. Also, there are some variants that we don't cover now that
probably would work as well by selecting an appropriate TARGET.

 Regards,
 Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera]

Hello Arnout,

On 23/06/16 21:17, Arnout Vandecappelle wrote:
[snip]
>>>>> On 2016-06-20 17:45 +0100, Vicente Olivert Riera spake thusly:
>>> [snip]
>>>>>> +config BR2_PACKAGE_OPENBLAS_TARGET
>>>>>> +	string "OpenBLAS target CPU"
>>>
>>>  This smells fishy to me. Why do we ask the user to fill this in? I mean, if
>>> we're building for power4, does it make sense to set this to e.g. CORE2?
>>>
>>>  In other words, I think this should be a blind option.
>>
>> there are OpenBLAS targets that apply for the same core. For instance:
>>
>> - KATMAI and COPPERMINE are pentium3.
>> - BANIAS and YONAH are pentium_m.
>> - CORE2, PENRYN and DUNNINGTON are core2.
> 
>  Where do you get this information?

KATMAI and COPPERMINE:
https://en.wikipedia.org/wiki/List_of_Intel_Pentium_III_microprocessors

For YONAH and DUNNINGTON I'm not 100% sure.

For CORE2 and PENRYN:
https://en.wikipedia.org/wiki/List_of_Intel_Core_2_microprocessors

>> So as per Thomas suggestion we choose one by default but let the user
>> change it.
> 
>  OK, I've taken a look at the TargetList.txt. I think actually there are not
> many more options than the ones you have listed now, so it _could_ be possible
> to add a choice for the missing ones. Alternatively, we could make the option
> visible only in the cases where there is ambiguity. However, both options would
> complicate things even more than they are now, and we're already at v6, so let's
> stick to the current approach. Still:
> 
> 1. The commit message should clarify this extensively.
> 2. The help text should explain more clearly what the user should choose.
> 
> Commit message:
> 
> OpenBLAS is optimised for specific CPU models, which don't fully match with the
> GCC code generation options. Therefore, we can't automatically select
> BR2_PACKAGE_OPENBLAS_TARGET based on the CPU choice. Instead, let the user
> select the TARGET name, but offer a sensible default. Other possible solutions
> were deemed too complicated: adding choice options in the ambiguous cases, or
> only making the option user-visible when there is ambiguity.
> 
> 
> Help text:
> 
> 	  OpenBLAS target CPU. See TargetList.txt in the source tree for the
> 	  possible target strings. A possible value is set automatically based
> 	  on your Target Architecture Variant.

Ok, I will use them in the next version.

> [snip]
>>>>> Is there an even lower "target CPU" that openBLAS knows of? Or should we
>>>>> just disable openBLAS for the aforementioned CPUs?
>>>>>
>>>>> Note: I haven't looked at the other archs, but arm springs to mind too
>>>>> (what would be the value for armv4, armv5 or armv6? Should it also be
>>>>> disabled for those as well?)
> 
>  It looks like armv4 is not supported and target should be set to ARMV5 or ARMV6
> as appropriate.

Ooops, I forgot about ARMV5 and ARMV6! Thanks.

Regards,

Vincent.

>>>>
>>>> So you want me to disable this package for every core we have in
>>>> Buildroot that doesn't have a matching OpenBLAS core?
>>>
>>>  That depends on whether OpenBLAS works on them or not. This is absolutely not
>>> clear from this patch. Please explain this in the commit log or in comments in
>>> your next version.
>>
>> Well, I think is sensible to enable OpenBLAS only for those BR cores who
>> have an usable OpenBLAS target.
> 
>  It would indeed be better that way, but again that would complicate things too
> much I think. Also, there are some variants that we don't cover now that
> probably would work as well by selecting an appropriate TARGET.
> 
>  Regards,
>  Arnout
> 
>