[OPENMP] Make -fopenmp to turn on OpenMP support by default.

[ABataev]

Summary

Patch turns on OpenMP support in clang by default after fixing OpenMP buildbots.

[chandlerc]

Some minor issues below. Also, Hans and Tom should have a glance at the release bits of this, I don't know any of that stuff.

Also, there are still some bugs with check-libomp. The immediate one I noticed was that when i run it in a clean build with enough parallelism, it fails to find the just-built clang binary. I suspect that the 'check-libomp' CMake target is missing dependencies on all of the binaries used when running the lit tests because it copied the libc++ CMake rules for it. While the libc++ CMake rules are *mostly* a good proxy, the *testing* strategy for libc++ is notably different here. In fact, libomp's testing is (IMO) quite a bit *better* here, and uncovers a nasty missing dependency.

The libc++ test suite tests libc++ with the *host* compiler, not the just-built compiler. This has some advantages and disadvantages, but for a library with close coupling to the compiler (such as compiler-rt or libomp) it is inappropriate. Your libomp tests very correctly use the just-built Clang, but the CMake needs to model this now. You can see in projects/compiler-rt/test/CMakeLists.txt lines 19-33 how compiler-rt sets up variables with these tools (clang, clang-headers, etc) which end up used on line 99 of projects/compiler-rt/test/asan/CMakeLists.txt for example.

I don't know what all tools you need (other than clang and probably clang-headers), probably not as many as the sanitizers do, so I'll let you put together a correct patch here. I've hacked around it locally to test further.

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.

configure

5953

You should be modifying the configure.ac file and regenerating this?

utils/llvm-lit/llvm-lit.in

42–46 ↗

(On Diff #37572)

This change is independent of enabling anything; can you break it out?

Also, manually patching this bit in order to test things didn't actually allow me to use llvm-lit with libomp, so I don't think this is quite working as intended yet.

[ABataev]

Hi Chandler, thanks for the review.

In D13802#272053, @chandlerc wrote:

Some minor issues below. Also, Hans and Tom should have a glance at the release bits of this, I don't know any of that stuff.

Also, there are still some bugs with check-libomp. The immediate one I noticed was that when i run it in a clean build with enough parallelism, it fails to find the just-built clang binary. I suspect that the 'check-libomp' CMake target is missing dependencies on all of the binaries used when running the lit tests because it copied the libc++ CMake rules for it. While the libc++ CMake rules are *mostly* a good proxy, the *testing* strategy for libc++ is notably different here. In fact, libomp's testing is (IMO) quite a bit *better* here, and uncovers a nasty missing dependency.

The libc++ test suite tests libc++ with the *host* compiler, not the just-built compiler. This has some advantages and disadvantages, but for a library with close coupling to the compiler (such as compiler-rt or libomp) it is inappropriate. Your libomp tests very correctly use the just-built Clang, but the CMake needs to model this now. You can see in projects/compiler-rt/test/CMakeLists.txt lines 19-33 how compiler-rt sets up variables with these tools (clang, clang-headers, etc) which end up used on line 99 of projects/compiler-rt/test/asan/CMakeLists.txt for example.

I don't know what all tools you need (other than clang and probably clang-headers), probably not as many as the sanitizers do, so I'll let you put together a correct patch here. I've hacked around it locally to test further.

I created a separate patch for libomp, that fixes this issue.

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.

Actually, these tests are written so, that they repeats about 1000 times to be sure that they are correct. But some of them might be sensible to system load.

configure

5953

Ok, done

utils/llvm-lit/llvm-lit.in

42–46 ↗

(On Diff #37572)

Ok, reverted it

[hans]

The release script change looks good to me. Better than good actually; it's a great improvement :-)

[chandlerc]

In D13802#274847, @ABataev wrote:

Hi Chandler, thanks for the review.

In D13802#272053, @chandlerc wrote:

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.

Actually, these tests are written so, that they repeats about 1000 times to be sure that they are correct. But some of them might be sensible to system load.

Hmm, so I think we need to find some way to make some of these tests more reliable.

With the fix from D14055 patched in (minus the flag change in it) I am seeing tests fail pretty regularly:

libomp :: worksharing/for/omp_for_schedule_auto.c

This test seems to fail pretty frequently for me. Maybe as much as half the time.

libomp :: worksharing/sections/omp_sections_nowait.c

This test fails less frequently, but still have seen it a few times.

libomp :: flush/omp_flush.c

I've seen this test fail just once...

[hfinkel]

In D13802#275471, @chandlerc wrote:

In D13802#274847, @ABataev wrote:

Hi Chandler, thanks for the review.

In D13802#272053, @chandlerc wrote:

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.

Actually, these tests are written so, that they repeats about 1000 times to be sure that they are correct. But some of them might be sensible to system load.

Hmm, so I think we need to find some way to make some of these tests more reliable.

With the fix from D14055 patched in (minus the flag change in it) I am seeing tests fail pretty regularly:

libomp :: worksharing/for/omp_for_schedule_auto.c

Credit goes to a colleague of mine, Ray Loy, for spotting this. This test has a race condition. The updates to the global sum1 are not protected by any kind of synchronization.

sum1 = sum0;

the for pragma is missing a private(sum1) clause?

This test seems to fail pretty frequently for me. Maybe as much as half the time.

libomp :: worksharing/sections/omp_sections_nowait.c

In omp_testsuite.h, we have this:

/* following times are in seconds */
#define SLEEPTIME 0.1

and this test, and also omp_flush.c, are sensitive to this. If the machine is heavily loaded, or otherwise configured, so the threads involved don't all get scheduled within 0.1 seconds, this will certainly fail.

Also, does this test have a race condition? Specifically, should there be a

#pragma omp flush(count)

before

if (count == 0)

I think there should be.

This test fails less frequently, but still have seen it a few times.

libomp :: flush/omp_flush.c

I've seen this test fail just once...

This is trying to test the memory flush pragma by having one thread write and flush, and a second thread wait for a fixed time, flush and read. As mentioned above, this is sensitive to SLEEPTIME.

[• jhowarth]

The proposed patches here to change the default behavior of -fopenmp from -fopenmp=libgomp to -fopenmp=libomp seem to only handle the configure-based build. The following change required to switch over the crake-based build to default -fopenmp to libomp is missing.

Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt	(revision 251539)
+++ CMakeLists.txt	(working copy)
@@ -196,7 +196,7 @@ set(GCC_INSTALL_PREFIX "" CACHE PATH "Di
 set(DEFAULT_SYSROOT "" CACHE PATH
   "Default <path> to all compiler invocations for --sysroot=<path>." )
 
-set(CLANG_DEFAULT_OPENMP_RUNTIME "libgomp" CACHE STRING
+set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 
 set(CLANG_VENDOR "" CACHE STRING

[ABataev]

In D13802#277270, @jhowarth wrote:

The proposed patches here to change the default behavior of -fopenmp from -fopenmp=libgomp to -fopenmp=libomp seem to only handle the configure-based build. The following change required to switch over the crake-based build to default -fopenmp to libomp is missing.

Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt	(revision 251539)
+++ CMakeLists.txt	(working copy)
@@ -196,7 +196,7 @@ set(GCC_INSTALL_PREFIX "" CACHE PATH "Di
 set(DEFAULT_SYSROOT "" CACHE PATH
   "Default <path> to all compiler invocations for --sysroot=<path>." )
 
-set(CLANG_DEFAULT_OPENMP_RUNTIME "libgomp" CACHE STRING
+set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 
 set(CLANG_VENDOR "" CACHE STRING

This patch is for clang and it is accepted already. But I'm going to land all patches for -fopenmp only when llvm and libomp changes are accepted.

[ABataev]

In D13802#276755, @hfinkel wrote:

In D13802#275471, @chandlerc wrote:

In D13802#274847, @ABataev wrote:

Hi Chandler, thanks for the review.

In D13802#272053, @chandlerc wrote:

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.

Actually, these tests are written so, that they repeats about 1000 times to be sure that they are correct. But some of them might be sensible to system load.

Hmm, so I think we need to find some way to make some of these tests more reliable.

With the fix from D14055 patched in (minus the flag change in it) I am seeing tests fail pretty regularly:

libomp :: worksharing/for/omp_for_schedule_auto.c

Credit goes to a colleague of mine, Ray Loy, for spotting this. This test has a race condition. The updates to the global sum1 are not protected by any kind of synchronization.

sum1 = sum0;

the for pragma is missing a private(sum1) clause?

This test seems to fail pretty frequently for me. Maybe as much as half the time.

libomp :: worksharing/sections/omp_sections_nowait.c

In omp_testsuite.h, we have this:

/* following times are in seconds */
#define SLEEPTIME 0.1

and this test, and also omp_flush.c, are sensitive to this. If the machine is heavily loaded, or otherwise configured, so the threads involved don't all get scheduled within 0.1 seconds, this will certainly fail.

Also, does this test have a race condition? Specifically, should there be a

#pragma omp flush(count)

before

if (count == 0)

I think there should be.

This test fails less frequently, but still have seen it a few times.

libomp :: flush/omp_flush.c

I've seen this test fail just once...

This is trying to test the memory flush pragma by having one thread write and flush, and a second thread wait for a fixed time, flush and read. As mentioned above, this is sensitive to SLEEPTIME.

Hal, thanks for pointing this. I'll take a look and try to fix these tests.

[• jhowarth]

The CMakeLists.txt change cited is in top-level of the clang sources and not in the llvm top-level.

[ABataev]

Chandler, gcc buildbot greenified, tests for clang are fixed. Hans Wennborg accepted this patch. Could accept this patch?

[• tstellarAMD]

Hans already commented on the release script changes, and I agrre they look good.

[hans]

Alexey & Chandler: Is there any ongoing communication here?

It would be great if we could resolve this, or at least figure out what the status is, before we branch for 3.8 in a month or so.

[ABataev]

Hi Hans,
I asked Chandler to look at this once again but seems to me he is very
busy. Maybe you could ping him also?

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

09.12.2015 0:35, Hans Wennborg пишет:

hans added a comment.

Alexey & Chandler: Is there any ongoing communication here?

It would be great if we could resolve this, or at least figure out what the status is, before we branch for 3.8 in a month or so.

http://reviews.llvm.org/D13802

[chandlerc]

Sorry I was a bit slow -- was out when commented in Nov, and was slow catching back up the past two weeks.

I've done some more testing, and the check-libomp is passing very nicely now! =D Ship it! Also make sure you follow up with the broader set of build bots to have them include this repository in the checkout.

Minor documentation tweak below, but feel free to land everything. =D I'm excited, it'll be interesting to see if anything else breaks, but so far it seems to be working well for me.

docs/GettingStarted.rst

58–62

I'd skip the optional tag -- as you've nicely documented, its required for OpenMP support.

[ABataev]

Chandler, thank you very much for review!

docs/GettingStarted.rst

58–62

Ok, removed

autoconf/configure.ac

AS_HELP_STRING([--with-default-sysroot],
     [Add --sysroot=<path> to all compiler invocations.]),,
     withval="")
 AC_DEFINE_UNQUOTED(DEFAULT_SYSROOT,"$withval",
                    [Default <path> to all compiler invocations for --sysroot=<path>.])
 
 AC_ARG_WITH(clang-default-openmp-runtime,
   AS_HELP_STRING([--with-clang-default-openmp-runtime],
     [The default OpenMP runtime for Clang.]),,
-    withval="libgomp")
+    withval="libomp")
 AC_DEFINE_UNQUOTED(CLANG_DEFAULT_OPENMP_RUNTIME,"$withval",
                    [Default OpenMP runtime used by -fopenmp.])
 
 dnl Allow linking of LLVM with GPLv3 binutils code.
 AC_ARG_WITH(binutils-include,
   AS_HELP_STRING([--with-binutils-include],
     [Specify path to binutils/include/ containing plugin-api.h file for gold plugin.]),,
   withval=default)

configure

 _ACEOF
 
 
 
 # Check whether --with-clang-default-openmp-runtime was given.
 if test "${with_clang_default_openmp_runtime+set}" = set; then
   withval=$with_clang_default_openmp_runtime;
 else
-  withval="libgomp"
+  withval="libomp"
 fi
 
 
 cat >>confdefs.h <<_ACEOF
 #define CLANG_DEFAULT_OPENMP_RUNTIME "$withval"
 _ACEOF
 
 

docs/GettingStarted.rst

#. Checkout Clang:
    * ``svn co http://llvm.org/svn/llvm-project/cfe/trunk clang``
 
 #. Checkout Compiler-RT (required to build the sanitizers):
 
    * ``cd where-you-want-llvm-to-live``
    * ``cd llvm/projects``
    * ``svn co http://llvm.org/svn/llvm-project/compiler-rt/trunk compiler-rt``
 
+#. Checkout Libomp (required for OpenMP support) **[Optional]**:
+
+   * ``cd where-you-want-llvm-to-live``
+   * ``cd llvm/projects``
+   * ``svn co http://llvm.org/svn/llvm-project/openmp/trunk openmp``
+
 #. Checkout libcxx and libcxxabi **[Optional]**:
 
    * ``cd where-you-want-llvm-to-live``
    * ``cd llvm/projects``
    * ``svn co http://llvm.org/svn/llvm-project/libcxx/trunk libcxx``
    * ``svn co http://llvm.org/svn/llvm-project/libcxxabi/trunk libcxxabi``
 
 #. Get the Test Suite Source Code **[Optional]**

 
 If you want to check out compiler-rt (required to build the sanitizers), run:
 
 .. code-block:: console
 
   % cd llvm/projects
   % git clone http://llvm.org/git/compiler-rt.git
 
+If you want to check out libomp (required for OpenMP support), run:
+
+.. code-block:: console
+
+  % cd llvm/projects
+  % git clone http://llvm.org/git/openmp.git
+
 If you want to check out libcxx and libcxxabi (optional), run:
 
 .. code-block:: console
 
   % cd llvm/projects
   % git clone http://llvm.org/git/libcxx.git
   % git clone http://llvm.org/git/libcxxabi.git
 
.. code-block:: console
   # If you have clang too:
   % cd tools
   % git clone http://llvm.org/git/clang.git
   % cd clang
   % git svn init https://llvm.org/svn/llvm-project/cfe/trunk --username=<username>
   % git config svn-remote.svn.fetch :refs/remotes/origin/master
   % git svn rebase -l
 
-Likewise for compiler-rt and test-suite.
+Likewise for compiler-rt, libomp and test-suite.
 
 To update this clone without generating git-svn tags that conflict with the
 upstream Git repo, run:
 
 .. code-block:: console
 
   % git fetch && (cd tools/clang && git fetch)  # Get matching revisions of both trees.
   % git checkout master
   % git svn rebase -l
   % (cd tools/clang &&
      git checkout master &&
      git svn rebase -l)
 
-Likewise for compiler-rt and test-suite.
+Likewise for compiler-rt, libomp and test-suite.
 
 This leaves your working directories on their master branches, so you'll need to
 ``checkout`` each working branch individually and ``rebase`` it on top of its
 parent branch.
 
 For those who wish to be able to update an llvm repo/revert patches easily using
 git-svn, please look in the directory for the scripts ``git-svnup`` and
 ``git-svnrevert``.

docs/ReleaseProcess.rst

 =======
 
 The scripts are in the ``utils/release`` directory.
 
 test-release.sh
 ---------------
 
 This script will check-out, configure and compile LLVM+Clang (+ most add-ons, like ``compiler-rt``,
-``libcxx`` and ``clang-extra-tools``) in three stages, and will test the final stage.
+``libcxx``, ``libomp`` and ``clang-extra-tools``) in three stages, and will test the final stage.
 It'll have installed the final binaries on the Phase3/Releasei(+Asserts) directory, and
 that's the one you should use for the test-suite and other external tests.
 
 To run the script on a specific release candidate run::
 
    ./test-release.sh \
         -release 3.3 \
         -rc 1 \

utils/release/test-release.sh

 do_checkout="yes"
 do_debug="no"
 do_asserts="no"
 do_compare="yes"
 do_rt="yes"
 do_libs="yes"
 do_libunwind="yes"
 do_test_suite="yes"
-do_openmp="no"
+do_openmp="yes"
 BuildDir="`pwd`"
 use_autoconf="no"
 ExtraConfigureFlags=""
 ExportBranch=""
 
 function usage() {
     echo "usage: `basename $0` -release X.Y.Z -rc NUM [OPTIONS]"
     echo ""
function usage() {
     echo " -configure-flags FLAGS  Extra flags to pass to the configure step."
     echo " -use-autoconf        Use autoconf instead of cmake"
     echo " -svn-path DIR        Use the specified DIR instead of a release."
     echo "                      For example -svn-path trunk or -svn-path branches/release_37"
     echo " -no-rt               Disable check-out & build Compiler-RT"
     echo " -no-libs             Disable check-out & build libcxx/libcxxabi/libunwind"
     echo " -no-libunwind        Disable check-out & build libunwind"
     echo " -no-test-suite       Disable check-out & build test-suite"
-    echo " -openmp              Check out and build the OpenMP run-time (experimental)"
+    echo " -no-openmp           Disable check-out & build libomp"
 }
 
 if [ `uname -s` = "Darwin" ]; then
   # compiler-rt doesn't yet build with CMake on Darwin.
   use_autoconf="yes"
 fi
 
 while [ $# -gt 0 ]; do
case $1 in
             do_libs="no"
             ;;
         -no-libunwind )
             do_libunwind="no"
             ;;
         -no-test-suite )
             do_test_suite="no"
             ;;
-        -openmp )
-            do_openmp="yes"
+        -no-openmp )
+            do_openmp="no"
             ;;
         -help | --help | -h | --h | -\? )
             usage
             exit 0
             ;;
         * )
             echo "unknown option: $1"
             usage
function export_sources() {
     fi
     cd $BuildDir/llvm.src/projects
     if [ -d $BuildDir/test-suite.src ] && [ ! -h test-suite ]; then
         ln -s ../../test-suite.src test-suite
     fi
     if [ -d $BuildDir/compiler-rt.src ] && [ ! -h compiler-rt ]; then
         ln -s ../../compiler-rt.src compiler-rt
     fi
+    if [ -d $BuildDir/openmp.src ] && [ ! -h openmp ]; then
+        ln -s ../../openmp.src openmp
+    fi
     if [ -d $BuildDir/libcxx.src ] && [ ! -h libcxx ]; then
         ln -s ../../libcxx.src libcxx
     fi
     if [ -d $BuildDir/libcxxabi.src ] && [ ! -h libcxxabi ]; then
         ln -s ../../libcxxabi.src libcxxabi
     fi
     if [ -d $BuildDir/libunwind.src ] && [ ! -h libunwind ]; then
         ln -s ../../libunwind.src libunwind
if [ "$use_gzip" = "yes" ]; then
       tar cfz $BuildDir/$Package.tar.gz $Package
     else
       tar cfJ $BuildDir/$Package.tar.xz $Package
     fi
     mv $Package llvmCore-$Release-$RC.install/usr/local
     cd $cwd
 }
 
-# Build and package the OpenMP run-time. This is still experimental and not
-# meant for official testing in the release, but as a way for providing
-# binaries as a convenience to those who want to try it out.
-function build_OpenMP() {
-    cwd=`pwd`
-
-    rm -rf $BuildDir/Phase3/openmp
-    rm -rf $BuildDir/Phase3/openmp.install
-    mkdir -p $BuildDir/Phase3/openmp
-    cd $BuildDir/Phase3/openmp
-    clang=$BuildDir/Phase3/Release/llvmCore-$Release-$RC.install/usr/local/bin/clang
-
-    echo "#" cmake -DCMAKE_C_COMPILER=${clang} -DCMAKE_CXX_COMPILER=${clang}++ \
-            -DCMAKE_BUILD_TYPE=Release -DLIBOMP_MICRO_TESTS=on \
-            $BuildDir/openmp.src/runtime
-    cmake -DCMAKE_C_COMPILER=${clang} -DCMAKE_CXX_COMPILER=${clang}++ \
-            -DCMAKE_BUILD_TYPE=Release -DLIBOMP_MICRO_TESTS=on \
-            $BuildDir/openmp.src/runtime
-
-    echo "# Building OpenMP run-time"
-    echo "# ${MAKE} -j $NumJobs VERBOSE=1"
-    ${MAKE} -j $NumJobs VERBOSE=1
-    echo "# ${MAKE} libomp-micro-tests VERBOSE=1"
-    ${MAKE} libomp-micro-tests VERBOSE=1
-    echo "# ${MAKE} install DESTDIR=$BuildDir/Phase3/openmp.install"
-    ${MAKE} install DESTDIR=$BuildDir/Phase3/openmp.install
-
-    OpenMPPackage=OpenMP-$Release
-    if [ $RC != "final" ]; then
-        OpenMPPackage=$OpenMPPackage-$RC
-    fi
-    OpenMPPackage=$OpenMPPackage-$Triple
-
-    mv $BuildDir/Phase3/openmp.install/usr/local $BuildDir/$OpenMPPackage
-    cd $BuildDir
-    tar cvfJ $BuildDir/$OpenMPPackage.tar.xz $OpenMPPackage
-    mv $OpenMPPackage $BuildDir/Phase3/openmp.install/usr/local
-    cd $cwd
-}
-
 # Exit if any command fails
 # Note: pipefail is necessary for running build commands through
 # a pipe (i.e. it changes the output of ``false | tee /dev/null ; echo $?``)
 set -e
 set -o pipefail
 
 if [ "$do_checkout" = "yes" ]; then
     export_sources
if [ "$do_compare" = "yes" ]; then
             if ! cmp -s <(sed -e 's,Phase2,Phase3,g' $p2) <(sed -e '' $p3) \
                     16 16 ; then
                 echo "file `basename $p2` differs between phase 2 and phase 3"
             fi
         done
     fi
 done
 
-if [ $do_openmp = "yes" ]; then
-  build_OpenMP
-fi
-
 ) 2>&1 | tee $LogDir/testing.$Release-$RC.log
 
 package_release
 
 set +e
 
 # Woo hoo!
 echo "### Testing Finished ###"