Enable by default the _sync version of atomic operations on OS X. #633

bosilca · 2015-06-10T05:33:23Z

As most compilers gain support for builtin atomics I propose to switch the default Open MPI behavior with regard to atomics. Instead of relying on our own atomics, we should leverage what the compiler, or the OS, is supporting. This patch change the default behavior for builds on OSX or with a compiler supporting gcc-type atomic builtins.

This is a replacement for the now defunct #288.

@goodell and @hjelmn please review.

lanl-ompi · 2015-06-10T05:36:59Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_distcheck/35/

Build Log
last 20 lines

[...truncated 1006 lines...]
*** Java OSHMEM bindings
checking if want Java bindings... no

*** OpenSHMEM profiling
checking if pshmem will be enabled... yes (weak symbols supported)

*** Assembler
checking dependency style of gcc -std=gnu99... gcc3
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking for fgrep... /bin/grep -F
checking for OSX atomic support... checking libkern/OSAtomic.h usability... no
checking libkern/OSAtomic.h presence... no
checking for libkern/OSAtomic.h... no
no
checking for libkern/OSAtomic.h... (cached) no
configure: error: OSX builtin atomics requested but not found.
Build step 'Execute shell' marked build as failure
Setting status of 0392ef74da68a5e6143a0bf554b8f4ad733e0902 to FAILURE with url http://jenkins.open-mpi.org/job/ompi_master_pr_distcheck/35/ and message: Build finished.

Test FAILed.

lanl-ompi · 2015-06-10T05:39:57Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_cle5.2up02/125/

Build Log
last 20 lines

[...truncated 1006 lines...]
*** Java OSHMEM bindings
checking if want Java bindings... no

*** OpenSHMEM profiling
checking if pshmem will be enabled... yes (weak symbols supported)

*** Assembler
checking dependency style of gcc -std=gnu99... gcc3
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking for fgrep... /usr/bin/grep -F
checking for OSX atomic support... checking libkern/OSAtomic.h usability... no
checking libkern/OSAtomic.h presence... no
checking for libkern/OSAtomic.h... no
no
checking for libkern/OSAtomic.h... (cached) no
configure: error: OSX builtin atomics requested but not found.
Build step 'Execute shell' marked build as failure
Setting status of 0392ef74da68a5e6143a0bf554b8f4ad733e0902 to FAILURE with url http://jenkins.open-mpi.org/job/ompi_master_pr_cle5.2up02/125/ and message: Build finished.

Test FAILed.

mellanox-github · 2015-06-10T05:43:10Z

Refer to this link for build results (access rights to CI server needed):
http://bgate.mellanox.com/job/gh-ompi-master-pr/616/

Build Log
last 50 lines

[...truncated 1652 lines...]
checking for __attribute__(weak_alias)... yes
checking for __attribute__(destructor)... yes
checking for compiler familyid... 1
checking for compiler familyname... GNU
checking for compiler version... 263175
checking for compiler version_str... 4.4.7

*** Java compiler
checking OSX locations... not found
checking Linux locations... found (/usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0.x86_64/include)
checking --with-jdk-bindir value... sanity check ok (/usr/bin)
checking --with-jdk-headers value... sanity check ok (/usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0.x86_64/include)
checking for javac... /usr/bin/javac
checking for javah... /usr/bin/javah
checking for jar... /usr/bin/jar
checking jni.h usability... yes
checking jni.h presence... yes
checking for jni.h... yes
checking Java support available... yes

*** Java MPI bindings
checking if want Java bindings... no

*** Java OSHMEM bindings
checking if want Java bindings... no

*** OpenSHMEM profiling
checking if pshmem will be enabled... yes (weak symbols supported)

*** Assembler
checking dependency style of gcc -std=gnu99... gcc3
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking for fgrep... /bin/grep -F
checking for OSX atomic support... checking libkern/OSAtomic.h usability... no
checking libkern/OSAtomic.h presence... no
checking for libkern/OSAtomic.h... no
no
checking for libkern/OSAtomic.h... (cached) no
configure: error: OSX builtin atomics requested but not found.
Build step 'Execute shell' marked build as failure
[htmlpublisher] Archiving HTML reports...
[htmlpublisher] Archiving at BUILD level /var/lib/jenkins/jobs/gh-ompi-master-pr/workspace/cov_build to /var/lib/jenkins/jobs/gh-ompi-master-pr/builds/616/htmlreports/Coverity_Report
Setting commit status on GitHub for https://github.com/open-mpi/ompi/commit/f129f658b22b9e9915b5b251a9b95a757b804c6d
[BFA] Scanning build for known causes...
[BFA] No failure causes found
[BFA] Done. 0s
Setting status of 0392ef74da68a5e6143a0bf554b8f4ad733e0902 to FAILURE with url http://bgate.mellanox.com:8888/jenkins/job/gh-ompi-master-pr/616/ and message: 'Build finished.'
Using conext: Mellanox

lanl-ompi · 2015-06-10T07:33:41Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_distcheck/36/
Test PASSed.

lanl-ompi · 2015-06-10T07:34:23Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_cle5.2up02/126/
Test PASSed.

mellanox-github · 2015-06-10T07:43:27Z

Coverity found 812 errors for all
Coverity found 5 errors for oshmem
Coverity found 2 errors for fca
Coverity report at http://bgate.mellanox.com:8888/jenkins/job/gh-ompi-master-pr//617/Coverity_Report

Sent from GH.

mellanox-github · 2015-06-10T07:57:46Z

Refer to this link for build results (access rights to CI server needed):
http://bgate.mellanox.com/job/gh-ompi-master-pr/617/

goodell · 2015-06-12T22:02:30Z

config/opal_config_asm.m4

-       AC_DEFINE([OPAL_C_GCC_INLINE_ASSEMBLY], [1],
-         [Whether C compiler supports GCC style inline assembly])
+    opal_cv_asm_builtin="BUILTIN_NO"
+    if test "$opal_cv_asm_builtin" = "BUILTIN_NO" -a "$enable_builtin_atomics" = "yes" ; then


Isn't the first part of this test trivially true because of the line above? Is this just to match the style of the other if checks below so that the if block could be more easily moved around later?

yes the first part is trivial but as you noticed it keeps all the if blocks similar.

goodell · 2015-06-12T22:10:34Z

Two issues:

The individual commits don't really make sense on their own. They should all be squashed down into a single commit.
While the __sync_ builtin atomics are more likely to be implemented correctly than our own inline assembly, they all carry a full memory barrier semantic with them. This is no imposition on x86, since x86 atomics are always full barriers. But performance on other platforms (POWER, ARM, etc.) might suffer. We could probably do better if we were using the C11 atomics (they should be used in seq_cst mode), but I don't think we have a current implementation in OPAL for that.

goodell · 2015-06-15T18:40:01Z

@bosilca It looks like you made a bad rebase + push since I last commented. The PR now shows ~15 commits, most of which are not yours. What's going on here?

jsquyres · 2015-06-26T03:58:57Z

I helped @bosilca fix his rebasing problems today. Looks like alles gut now with this PR.

Merge?

jsquyres · 2015-08-15T13:17:48Z

@bosilca Looks like this PR also had a bad rebase (after our session that fixed the problems in SJC...). Can you fix? Do you want to webex where we can fix this together?

result messages

jsquyres · 2015-08-18T15:30:48Z

@bosilca Thanks for fixing / rebasing.

We talked about this on the call today; @hjelmn is going to re-review this one. He'll do some testing to see if the lack of a swap atomic is problematic, performance-wise.

bosilca · 2015-09-27T15:11:44Z

@hjelmn did you had the time to investigate the potential impact of the atomic swap?

hjelmn · 2015-09-28T01:19:32Z

Will check performance numbers tomorrow.

hjelmn · 2015-09-28T15:40:53Z

See no noticeable difference in performance with the builtins vs asm.

bosilca · 2015-09-28T16:03:03Z

I'll merge it then. Thanks.

Enable by default the _sync version of atomic operations on OS X.

bosilca added the enhancement label Jun 10, 2015

bosilca assigned goodell Jun 10, 2015

bosilca added this to the Open MPI v2.0.0 milestone Jun 10, 2015

bosilca mentioned this pull request Jun 10, 2015

Topic/enable atomics #288

Closed

goodell reviewed Jun 12, 2015
View reviewed changes

bosilca force-pushed the topic/enable_atomics branch from 8f35740 to 29321e0 Compare June 25, 2015 18:02

bosilca force-pushed the topic/enable_atomics branch from a38cf80 to 9c3db9a Compare August 17, 2015 20:53

bosilca added 3 commits August 17, 2015 18:08

Enable by default the _sync version of atomic operations on OS X.

3eaefd7

Clean-up the OS X selection logic.

5aadf39

Have the __sync atomics the default. Remove spurious checking and

277269b

result messages

bosilca force-pushed the topic/enable_atomics branch from 9c3db9a to 277269b Compare August 17, 2015 22:08

bosilca added a commit that referenced this pull request Sep 28, 2015

Merge pull request #633 from bosilca/topic/enable_atomics

984b35b

Enable by default the _sync version of atomic operations on OS X.

bosilca merged commit 984b35b into open-mpi:master Sep 28, 2015

bosilca deleted the topic/enable_atomics branch September 28, 2015 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable by default the _sync version of atomic operations on OS X. #633

Enable by default the _sync version of atomic operations on OS X. #633

bosilca commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

goodell Jun 12, 2015

bosilca Jun 14, 2015

goodell commented Jun 12, 2015

goodell commented Jun 15, 2015

jsquyres commented Jun 26, 2015

jsquyres commented Aug 15, 2015

jsquyres commented Aug 18, 2015

bosilca commented Sep 27, 2015

hjelmn commented Sep 28, 2015

hjelmn commented Sep 28, 2015

bosilca commented Sep 28, 2015

Enable by default the _sync version of atomic operations on OS X. #633

Enable by default the _sync version of atomic operations on OS X. #633

Conversation

bosilca commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

lanl-ompi commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

mellanox-github commented Jun 10, 2015

goodell Jun 12, 2015

Choose a reason for hiding this comment

bosilca Jun 14, 2015

Choose a reason for hiding this comment

goodell commented Jun 12, 2015

goodell commented Jun 15, 2015

jsquyres commented Jun 26, 2015

jsquyres commented Aug 15, 2015

jsquyres commented Aug 18, 2015

bosilca commented Sep 27, 2015

hjelmn commented Sep 28, 2015

hjelmn commented Sep 28, 2015

bosilca commented Sep 28, 2015