Re: detection and support of OpenMP

[Ralf Wildenhues]

Hello Bruno,

Thanks for the patch.  I'll let someone else decide over the general
questions (whether this should be in Autoconf etc.), and just give a bunch
of random thoughts:

* Bruno Haible wrote on Thu, May 17, 2007 at 12:30:31PM CEST:
>
> Therefore here is an autoconf addition that makes it easier for a
> developer to use OpenMP.
> The macro has been tested for a long time in GNU gettext.

It seems to me this macro does not actually check whether OpenMP code
would be accepted by the compiler, nor whether any necessary runtime
libraries are actually linked in.  Using an OpenMP function (or, even
better, linking a small portable self-contained OpenMP program) could
achieve this.

What if I want to create Fortran or C++ code that uses OpenMP?

While I understand why you choose to set OPENMP_CFLAGS rather than
amending CFLAGS, and it makes sense to me, too, it does not fit the other
modes of operation of Autoconf macros.  Not sure if this
inconsistency in Autoconf weighs less than the advantage.  Intruding into
users' namespace is always a double-edged sword.

> In the macro, it would have been simpler to blindly test for the first
> supported option in a list "-fopenmp -xopenmp -openmp -mp -omp -qsmp=omp",
> but many compilers accept several of these options and do something
> unrelated - like to create an output file called 'penmp' or to warn
> about an unsupported compiler option. The code therefore applies each
> option only to the brand of compiler that is known to be likely to
> support it.

Another way to address this would be to enter a subdir `conftest' and
ensure no file with such a name is created.

> In the doc, I'm not sure whether one should write "The
> @code{AC_C_OPENMP} macro" or "The macro @code{AC_C_OPENMP}". Please
> correct if needed.

FWIW, I'd slightly prefer the latter.

> 2007-05-17  Bruno Haible  <address@hidden>
>
>       * lib/autoconf/c.m4 (AC_C_OPENMP): New macro.
>       * doc/autoconf.texi (C Compiler): Document AC_C_OPENMP.

This change would need to be mentioned in NEWS.

> + AC_DEFUN([AC_C_OPENMP],
> + [
> +   AC_MSG_CHECKING([whether to use OpenMP])
> +   AC_ARG_ENABLE(openmp,
> +     [  --disable-openmp        do not use OpenMP],

The enable switch is not documented in the manual.
Please use AS_HELP_STRING.

> +     [ac_openmp_choice="$enableval"],

Superfluous double-quotes.

> +     [ac_openmp_choice=yes])
> +   AC_MSG_RESULT([$ac_openmp_choice])
> +   OPENMP_CFLAGS=
> +   if test "$ac_openmp_choice" = yes; then
> +     AC_MSG_CHECKING([for $CC option to support OpenMP])
> +     AC_CACHE_VAL([ac_cv_prog_cc_openmp], [
> +       ac_cv_prog_cc_openmp=no
> +       AC_COMPILE_IFELSE([
> + #ifndef _OPENMP
> +  Unlucky
> + #endif
> +         ], [ac_cv_prog_cc_openmp=none])
> +       if test "$ac_cv_prog_cc_openmp" = no; then
> +         dnl Try these flags:
> +         dnl   GCC >= 4.2           -fopenmp
> +         dnl   SunPRO C             -xopenmp
> +         dnl   Intel C              -openmp
> +         dnl   SGI C, PGI C         -mp
> +         dnl   Tru64 Compaq C       -omp
> +         dnl   AIX IBM C            -qsmp=omp

Doesn't AIX have a compiler for GNU/Linux as well?  Does it support OpenMP?

The Intel compiler can fool the $GCC test with suitable compiler options
(no arms race between the Intel people making icc more like GCC and
Autoconf detecting non-GCC compilers, please; this holds for some other
compilers as well), but at least the version I just tried (8.1) does not
understand -fopenmp, warns about it, but does not fail.  Dunno if that
would be enough of a reason to try Intel first?

> +         if test "$GCC" = yes; then
> +           dnl --- Test for GCC.
> +           gt_save_CFLAGS="$CFLAGS"

s/gt_/ac_/g  here and elsewhere.  Superfluous double-quotes here.

> +           CFLAGS="$CFLAGS -fopenmp"
> +           AC_COMPILE_IFELSE([
> + #ifndef _OPENMP
> +  Unlucky

Please use 'choke me'.

> + #endif
> +             ], [ac_cv_prog_cc_openmp="-fopenmp"])
[...]

Can we eliminate some of the redundancy in the code by a loop like this,
if we decide to go the way of using preprocessor defines?
Besides cutting down the size of the expanded macro quite a bit it would
also help avoid inconsistencies when the macro is bugfixed later.
Also, per comment above, why not make a link test?
(Completely untested code follows.)

  # do GCC test beforehand...

  ac_omp_flag=
  for ac_omp_cpp in \
    -xopenmp 'defined __SUNPRO_C || defined __SUNPRO_CC' \
    -openmp 'defined __INTEL_COMPILER' \
    -mp 'defined __sgi || defined __PGI || defined __PGIC_' \
    -omp 'defined __DECC || defined __DECCXX' \
    -qsmp=omp 'defined _AIX' \
  do
    if test -z "ac_omp_flag"; then
      ac_omp_flag=$ac_omp_cpp
      continue
    fi
    AC_EGREP_CPP([Brand], [[
#if $ac_omp_cpp
 Brand
#endif
      ]], [ac_openmp_result=yes], [ac_openmp_result=no])
    if test $ac_openmp_result = yes; then
      ac_save_CFLAGS=$CFLAGS
      CFLAGS="$CFLAGS $ac_omp_flag"
      AC_LINK_IFELSE([[
#ifndef _OPENMP
 choke me
#else
 /* a small openmp test here...*/
#endif
      ]], [ac_cv_prog_cc_openmp=$ac_omp_flag])

    test "$ac_cv_prog_cc_openmp" != "none needed" && break
    ac_omp_flag=
  fi

> +     case $ac_cv_prog_cc_openmp in
> +       none)

Why not just initialize $ac_cv_prog_cc_openmp with "none needed",
"unsupported" etc. right away?

> +         AC_MSG_RESULT([none needed]) ;;
> +       no)
> +         AC_MSG_RESULT([unsupported]) ;;
> +       *)
> +         AC_MSG_RESULT([$ac_cv_prog_cc_openmp]) ;;
[...]

Please ensure that the generated test for this new macro succeeds.
You can run it verbosely like this:
  make check TESTSUITEFLAGS='-k AC_C_OPENMP -v -d -x'

Cheers,
Ralf