[Buildroot] [PATCH v6] openblas: new package
Vicente Olivert Riera
Vincent.Riera at imgtec.com
Thu Jun 23 09:30:56 UTC 2016
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
>>
>
>
More information about the buildroot
mailing list