Patchwork beecrypt: enable OpenMP if Buildroot GCC supports it

login
register
mail settings
Submitter Stefan Fröberg
Date Oct. 12, 2012, 7:54 p.m.
Message ID <1350071690-11279-1-git-send-email-stefan.froberg@petroprogram.com>
Download mbox | patch
Permalink /patch/191186/
State Rejected
Headers show

Comments

Stefan Fröberg - Oct. 12, 2012, 7:54 p.m.
Signed-off-by: Stefan Fröberg <stefan.froberg@petroprogram.com>
---
 package/beecrypt/beecrypt.mk |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Arnout Vandecappelle - Oct. 13, 2012, 9:29 a.m.
On 12/10/12 21:54, Stefan Fröberg wrote:
>
> Signed-off-by: Stefan Fröberg<stefan.froberg@petroprogram.com>
> ---
>   package/beecrypt/beecrypt.mk |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/package/beecrypt/beecrypt.mk b/package/beecrypt/beecrypt.mk
> index d165aee..101d0df 100644
> --- a/package/beecrypt/beecrypt.mk
> +++ b/package/beecrypt/beecrypt.mk
> @@ -18,6 +18,12 @@ BEECRYPT_CONF_OPT = \
>   		--without-java \
>   		--without-python
>
> +ifeq ($(BR2_GCC_ENABLE_OPENMP),y)
> +BEECRYPT_CONF_OPT += --enable-openmp
> +else
> +BEECRYPT_CONF_OPT += --disable-openmp
> +endif
> +

  This doesn't solve the underlying issue that -lgomp isn't included.  In
addition, it would only enable openmp for internal toolchains, while it may
be present for ct-ng or external toolchains as well.  I think the automatic
detection works well in this case.

  Regards,
  Arnout
Stefan Fröberg - Oct. 13, 2012, 1:25 p.m.
13.10.2012 12:29, Arnout Vandecappelle kirjoitti:
> On 12/10/12 21:54, Stefan Fröberg wrote:
>>
>> Signed-off-by: Stefan Fröberg<stefan.froberg@petroprogram.com>
>> ---
>>   package/beecrypt/beecrypt.mk |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/package/beecrypt/beecrypt.mk b/package/beecrypt/beecrypt.mk
>> index d165aee..101d0df 100644
>> --- a/package/beecrypt/beecrypt.mk
>> +++ b/package/beecrypt/beecrypt.mk
>> @@ -18,6 +18,12 @@ BEECRYPT_CONF_OPT = \
>>           --without-java \
>>           --without-python
>>
>> +ifeq ($(BR2_GCC_ENABLE_OPENMP),y)
>> +BEECRYPT_CONF_OPT += --enable-openmp
>> +else
>> +BEECRYPT_CONF_OPT += --disable-openmp
>> +endif
>> +
>
>  This doesn't solve the underlying issue that -lgomp isn't included.  In
> addition, it would only enable openmp for internal toolchains, while
> it may
> be present for ct-ng or external toolchains as well.  I think the
> automatic
> detection works well in this case.
>

Yeah, but how do you test the presence of OpenMP in case of external
toolchain ?
Checking if /usr/lib/libgomp.so.* are present ?
Parsing the output of external toolchain "gcc -v" ? (there is not even
configure switch for OpenMP in current gcc)

There are just too many variables and corner cases when it comes for
testing various features of buildroot external toolchains.

And the automatic detection didn't work for Alexander and he was even
using the internal toolchain
(if Im not wrong he uses ARM version of internal buildroot toolchain ?)


Best regards
Stefan
>  Regards,
>  Arnout
Arnout Vandecappelle - Oct. 13, 2012, 1:29 p.m.
On 13/10/12 15:25, Stefan Fröberg wrote:
> And the automatic detection didn't work for Alexander and he was even
> using the internal toolchain

  The automatic detection did work (and it's part of autoconf, BTW).  However,
beecrypt uses the OPENMP_CFLAGS given by autoconf to determine if OpenMP is
available or not - but OPENMP_CFLAGS may be empty if OpenMP is already enabled
without any CFLAGS.  So it should instead check on ac_cv_prog_c_openmp.

  Regards,
  Arnout
Stefan Fröberg - Oct. 13, 2012, 2:07 p.m.
13.10.2012 16:29, Arnout Vandecappelle kirjoitti:
> On 13/10/12 15:25, Stefan Fröberg wrote:
>> And the automatic detection didn't work for Alexander and he was even
>> using the internal toolchain
>
>  The automatic detection did work (and it's part of autoconf, BTW). 
> However,
> beecrypt uses the OPENMP_CFLAGS given by autoconf to determine if
> OpenMP is
> available or not - but *OPENMP_CFLAGS may be empty if OpenMP is
> already enabled
> without any CFLAGS*.  So it should instead check on ac_cv_prog_c_openmp.
>
>  Regards,
>  Arnout

Isn't that ac_cv_prog_c_openmp in configure file the same as AC_OPENMP
macro ?

And from:
https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Generic-Compiler-Characteristics.html

/"If the current language is C, the macro //|AC_OPENMP|//sets the
variable //|OPENMP_CFLAGS|//to the C compiler flags needed for
supporting OpenMP. /*/|OPENMP_CFLAGS|/**/is set to empty if the compiler
already supports OpenMP, if it has no way to activate OpenMP support, or
if the user rejects OpenMP support by invoking ‘/**/configure/**/’ with
the ‘/**/--disable-openmp/*/*’ option.* //"//
/
So from that:
1 ) If compiler *supports* OpenMP it set's  OPENMP_CFLAGS to empty. No
OpenMP for you.
2 ) If compiler has no clue how to activate OpenMP, it will set
OPENMP_CFLAGS to empty. No OpenMP for you.
3 ) If user refuses to use OpenMP with --disable-openmp (like in that my
patch) it will set OPENMP_CFLAGS to empty. No OpenMP for you.


Quick grepping of beecrypt-4.2.1 configure file shows that   
OPENMP_CFLAGS=ac_cv_prog_c_openmp

grep -n ac_cv_prog_c configure

configure:17706     OPENMP_CFLAGS=$ac_cv_prog_c_openmp ;;

This all sounds soooo messy.

Regards:
Stefan
Alexander Khryukin - Oct. 13, 2012, 2:54 p.m.
2012/10/13 Stefan Fröberg <stefan.froberg@petroprogram.com>

>  13.10.2012 16:29, Arnout Vandecappelle kirjoitti:
>
> On 13/10/12 15:25, Stefan Fröberg wrote:
>
> And the automatic detection didn't work for Alexander and he was even
> using the internal toolchain
>
>
>  The automatic detection did work (and it's part of autoconf, BTW).
> However,
> beecrypt uses the OPENMP_CFLAGS given by autoconf to determine if OpenMP
> is
> available or not - but *OPENMP_CFLAGS may be empty if OpenMP is already
> enabled
> without any CFLAGS*.  So it should instead check on ac_cv_prog_c_openmp.
>
>  Regards,
>  Arnout
>
>
> Isn't that ac_cv_prog_c_openmp in configure file the same as AC_OPENMP
> macro ?
>
> And from:
>
> https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Generic-Compiler-Characteristics.html
>
> *"If the current language is C, the macro **AC_OPENMP** sets the variable
> **OPENMP_CFLAGS** to the C compiler flags needed for supporting OpenMP. **
> OPENMP_CFLAGS** is set to empty if the compiler already supports OpenMP,
> if it has no way to activate OpenMP support, or if the user rejects OpenMP
> support by invoking ‘**configure**’ with the ‘**--disable-openmp**’
> option. **"**
> *
> So from that:
> 1 ) If compiler *supports* OpenMP it set's  OPENMP_CFLAGS to empty. No
> OpenMP for you.
> 2 ) If compiler has no clue how to activate OpenMP, it will set
> OPENMP_CFLAGS to empty. No OpenMP for you.
> 3 ) If user refuses to use OpenMP with --disable-openmp (like in that my
> patch) it will set OPENMP_CFLAGS to empty. No OpenMP for you.
>
>
> Quick grepping of beecrypt-4.2.1 configure file shows that
> OPENMP_CFLAGS=ac_cv_prog_c_openmp
>
> grep -n ac_cv_prog_c configure
>
> configure:17706     OPENMP_CFLAGS=$ac_cv_prog_c_openmp ;;
>
> This all sounds soooo messy.
>
> Regards:
> Stefan
>
>
>
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>


Use my patch please

http://pastie.org/pastes/5052617/text
Stefan Fröberg - Oct. 13, 2012, 3:07 p.m.
13.10.2012 17:54, Alexander Khryukin kirjoitti:
>
>
> 2012/10/13 Stefan Fröberg <stefan.froberg@petroprogram.com
> <mailto:stefan.froberg@petroprogram.com>>
>
>     13.10.2012 16:29, Arnout Vandecappelle kirjoitti:
>>     On 13/10/12 15:25, Stefan Fröberg wrote:
>>>     And the automatic detection didn't work for Alexander and he was
>>>     even
>>>     using the internal toolchain
>>
>>      The automatic detection did work (and it's part of autoconf,
>>     BTW).  However,
>>     beecrypt uses the OPENMP_CFLAGS given by autoconf to determine if
>>     OpenMP is
>>     available or not - but *OPENMP_CFLAGS may be empty if OpenMP is
>>     already enabled
>>     without any CFLAGS*.  So it should instead check on
>>     ac_cv_prog_c_openmp.
>>
>>      Regards,
>>      Arnout
>
>     Isn't that ac_cv_prog_c_openmp in configure file the same as
>     AC_OPENMP macro ?
>
>     And from:
>     https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Generic-Compiler-Characteristics.html
>
>     /"If the current language is C, the macro //|AC_OPENMP|//sets the
>     variable //|OPENMP_CFLAGS|//to the C compiler flags needed for
>     supporting OpenMP. /*/|OPENMP_CFLAGS|/**/is set to empty if the
>     compiler already supports OpenMP, if it has no way to activate
>     OpenMP support, or if the user rejects OpenMP support by invoking
>     ‘/**/configure/**/’ with the ‘/**/--disable-openmp/*/*’ option.* //"//
>     /
>     So from that:
>     1 ) If compiler *supports* OpenMP it set's  OPENMP_CFLAGS to
>     empty. No OpenMP for you.
>     2 ) If compiler has no clue how to activate OpenMP, it will set
>     OPENMP_CFLAGS to empty. No OpenMP for you.
>     3 ) If user refuses to use OpenMP with --disable-openmp (like in
>     that my patch) it will set OPENMP_CFLAGS to empty. No OpenMP for you.
>
>
>     Quick grepping of beecrypt-4.2.1 configure file shows that   
>     OPENMP_CFLAGS=ac_cv_prog_c_openmp
>
>     grep -n ac_cv_prog_c configure
>
>     configure:17706     OPENMP_CFLAGS=$ac_cv_prog_c_openmp ;;
>
>     This all sounds soooo messy.
>
>     Regards:
>     Stefan
>
>
>
>     _______________________________________________
>     buildroot mailing list
>     buildroot@busybox.net <mailto:buildroot@busybox.net>
>     http://lists.busybox.net/mailman/listinfo/buildroot
>
>
>
> Use my patch please
>
> http://pastie.org/pastes/5052617/text
>
I would but your patch forces the use of OpenMP in beecrypt, even in
case when user's gcc does not have that for his/hers gcc.
So it will end to link error for those users.

So the beecrypt should have handled the OpenMP support by autodetecting
it (and you do have OpenMP in your gcc) in configure
process. But it did not.
And Im curious to know why.

Maybe someone could show what that mysterious AC_OPENMP actually does ...

Regards
Stefan
Arnout Vandecappelle - Oct. 13, 2012, 8:56 p.m.
On 13/10/12 16:54, Alexander Khryukin wrote:
> Use my patch please
>
> http://pastie.org/pastes/5052617/text

  That patch adds -lgomp unconditionally, even if OpenMP is not available.

  Regards,
  Arnout

Patch

diff --git a/package/beecrypt/beecrypt.mk b/package/beecrypt/beecrypt.mk
index d165aee..101d0df 100644
--- a/package/beecrypt/beecrypt.mk
+++ b/package/beecrypt/beecrypt.mk
@@ -18,6 +18,12 @@  BEECRYPT_CONF_OPT = \
 		--without-java \
 		--without-python
 
+ifeq ($(BR2_GCC_ENABLE_OPENMP),y)
+BEECRYPT_CONF_OPT += --enable-openmp
+else
+BEECRYPT_CONF_OPT += --disable-openmp
+endif
+
 ifeq ($(BR2_PACKAGE_ICU),y)
 # C++ support needs icu
 BEECRYPT_DEPENDENCIES += icu