[Buildroot] [PATCH v6] openblas: new package

[Vicente Olivert Riera <Vincent.Riera@imgtec.com>]

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
>>
> 
>