Page MenuHomePhabricator

[mips] Use libatomic instead of GCC intrinsics for 64bit
ClosedPublic

Authored by mstojanovic on Apr 16 2018, 8:28 AM.

Details

Summary

The following GCC intrinsics are not available on MIPS32:

__sync_fetch_and_add_8
__sync_fetch_and_and_8
__sync_fetch_and_or_8
__sync_val_compare_and_swap_8

This patch is replacing these with appropriate libatomic implementation.

Patch by Miodrag Dinic <miodrag.dinic@mips.com>

Diff Detail

Repository
rL LLVM

Event Timeline

mstojanovic created this revision.Apr 16 2018, 8:28 AM
mstojanovic edited the summary of this revision. (Show Details)Apr 16 2018, 8:29 AM
miodrag.dinic edited the summary of this revision. (Show Details)Apr 16 2018, 8:35 AM

Can you re-upload this with full context?

One major comment inlined, I'd like one of the other reviewers to chime in if it's the better solution.

I'm also seeing linker errors relating to atomic_compare_exchange_8, atomic_compare_add_8 when building the target 'omp' with an in-tree build. This is due to -Wl,--as-needed and the ordering of -latomic w.r.t. the objects and output.

Removing --as-needed or adding -latomic to the end of the linker-via-compiler command resolves the issue, but I think the more correct solution is to detect when libatomic is required, (like libcxx) then add libatomic as one of the libraries when required.

runtime/src/kmp_os.h
492 ↗(On Diff #142794)

I don't think we should pollute the definitions here with a series of

#if defined(KMP_ARCH_MIPS)

conditionals. Instead I would suggest defining the likes of KMP_TEST_THEN_INC64(p) and the other 64 bit atomics as

#define KMP_TEST_THEN_INC64(p) KMP_TEST_THEN_INC64_IMPL(p)

Where the likes of KMP_TEST_THEN_INC64_IMPL is defined before these macros under a single #if like:

#if !defined(KMP_ARCH_MIPS)
  #define KMP_TEST_THEN_INC64_IMPL(p) __atomic_fetch_add((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
  ....
#elif defined(KMP_ARCH_MIPS)
  #define KMP_TEST_THEN_INC64_IMPL(p) __sync_fetch_and_add((volatile kmp_int64 *)(p), 1LL)
  ...
#else
  #error "Unknown architecture OMP runtime!"
#endif

With the necessary MIPS specific code included in that #elif branch. That conditional define will need a comment explaining that MIPS32 lacks 64bit atomics and so must use libatomic's implementation of those functions.

Hahnfeld edited subscribers, added: openmp-commits; removed: llvm-commits.Apr 19 2018, 5:43 AM

Ideally, this problem was already solved by other standards. Can we reuse C11 or even C++ atomics? In the future this would also eliminate memory barriers, but such change will require some amount of thinking and testing.

@mstojanovic @sdardis Could you test on MIPS after D47903 landed? The runtime is now using C++11 <atomic> which should be portable across platforms.

@Hahnfeld As it stands the issue is still present on MIPS. Is there a way to use D47903 in this context?

@Hahnfeld As it stands the issue is still present on MIPS. Is there a way to use D47903 in this context?

I'm not sure I understand: Does that mean even latest trunk (which includes the mentioned change) doesn't compile on MIPS?

Yes, is still runs into liker errors with the trunk version.

Ok, it looks like D47903 didn't convert all code to std::atomic. In the long term that's probably what we want. @jlpeyton are you (Intel) already working on this?

Ok, it looks like D47903 didn't convert all code to std::atomic. In the long term that's probably what we want. @jlpeyton are you (Intel) already working on this?

We are not currently working on this and unfortunately we do not have time to work on this at present, but would be more than welcome to receive patches from the community.

Sorry for the delay!

Ok, it looks like D47903 didn't convert all code to std::atomic. In the long term that's probably what we want. @jlpeyton are you (Intel) already working on this?

We are not currently working on this and unfortunately we do not have time to work on this at present, but would be more than welcome to receive patches from the community.

Given that situation and the scope of using C++11 atomics everywhere, I think it's reasonable to take this fix in its current form. Opinions?

runtime/src/kmp_os.h
492 ↗(On Diff #142794)

Sounds reasonable. On the other hand the same pattern is used for KMP_COMPARE_AND_STORE_PTR... In any case I think the current order should be preserved, in particular KMP_TEST_THEN_DEC32 before KMP_TEST_THEN_DEC64.

AndreyChurbanov requested changes to this revision.Mar 5 2019, 8:54 AM
AndreyChurbanov added inline comments.
runtime/src/kmp_os.h
605 ↗(On Diff #142794)

This looks wrong, because the function (or macro) should return the value of *p before the CAS operation, so it should return cv I think, which is either expected value of *p in case of success, or unexpected value read from *p in case CAS operation failed.

This revision now requires changes to proceed.Mar 5 2019, 8:54 AM

Changed mips_sync_val_compare_and_swap to return cv.

This revision is now accepted and ready to land.Mar 7 2019, 2:22 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2019, 2:52 AM

Please note this change set broke the following buildbot and has yet to be fixed: http://lab.llvm.org:8011/builders/openmp-clang-ppc64le-linux-rhel/builds/140

Please note this change set broke the following buildbot and has yet to be fixed: http://lab.llvm.org:8011/builders/openmp-clang-ppc64le-linux-rhel/builds/140

I don't think that's true, did you look at the error message?

-- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR
-- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR - Failed
CMake Error at cmake/modules/CheckCompilerVersion.cmake:102 (message):
  libstdc++ version should be at least 5.1 because LLVM will soon use new C++
  features which your toolchain version doesn't support.  You can temporarily
  opt out using LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN, but very soon your
  toolchain won't be supported.
Call Stack (most recent call first):
  cmake/config-ix.cmake:13 (include)
  CMakeLists.txt:629 (include)

This is when compiling Clang and I'd guess that's because of https://github.com/llvm/llvm-project/commit/51dcfdbba334fbd6c84fdf652023106d6714c73b. This problem needs to be fixed by the bot owner!

Please note this change set broke the following buildbot and has yet to be fixed: http://lab.llvm.org:8011/builders/openmp-clang-ppc64le-linux-rhel/builds/140

I don't think that's true, did you look at the error message?

-- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR
-- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR - Failed
CMake Error at cmake/modules/CheckCompilerVersion.cmake:102 (message):
  libstdc++ version should be at least 5.1 because LLVM will soon use new C++
  features which your toolchain version doesn't support.  You can temporarily
  opt out using LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN, but very soon your
  toolchain won't be supported.
Call Stack (most recent call first):
  cmake/config-ix.cmake:13 (include)
  CMakeLists.txt:629 (include)

This is when compiling Clang and I'd guess that's because of https://github.com/llvm/llvm-project/commit/51dcfdbba334fbd6c84fdf652023106d6714c73b. This problem needs to be fixed by the bot owner!

Whoops sorry about this. I have notified the bot owner thanks!