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

[mstojanovic]

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 <[e-mail]>

[sdardis]

Can you re-upload this with full context?

[sdardis]

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]

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.

[Hahnfeld]

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

[mstojanovic]

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

[Hahnfeld]

In D45691#1208115, @mstojanovic wrote:

@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?

[mstojanovic]

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

[Hahnfeld]

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?

[jlpeyton]

In D45691#1210590, @Hahnfeld wrote:

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.

[Hahnfeld]

Sorry for the delay!

In D45691#1210989, @jlpeyton wrote:

In D45691#1210590, @Hahnfeld wrote:

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]

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.

[mstojanovic]

Changed mips_sync_val_compare_and_swap to return cv.

[AndreyChurbanov]

LGTM

[patricklyster]

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

[Hahnfeld]

In D45691#1427670, @patricklyster wrote:

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!

[patricklyster]

In D45691#1427675, @Hahnfeld wrote:

In D45691#1427670, @patricklyster wrote:

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!

openmp/trunk/runtime/src/kmp_os.h

 
 #elif (KMP_ASM_INTRINS && KMP_OS_UNIX) || !(KMP_ARCH_X86 || KMP_ARCH_X86_64)
 
 /* cast p to correct type so that proper intrinsic will be used */
 #define KMP_TEST_THEN_INC32(p)                                                 \
   __sync_fetch_and_add((volatile kmp_int32 *)(p), 1)
 #define KMP_TEST_THEN_INC_ACQ32(p)                                             \
   __sync_fetch_and_add((volatile kmp_int32 *)(p), 1)
+#if defined(KMP_ARCH_MIPS)
+#define KMP_TEST_THEN_INC64(p)                                                 \
+  __atomic_fetch_add((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
+#define KMP_TEST_THEN_INC_ACQ64(p)                                             \
+  __atomic_fetch_add((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
+#else
 #define KMP_TEST_THEN_INC64(p)                                                 \
   __sync_fetch_and_add((volatile kmp_int64 *)(p), 1LL)
 #define KMP_TEST_THEN_INC_ACQ64(p)                                             \
   __sync_fetch_and_add((volatile kmp_int64 *)(p), 1LL)
+#endif
 #define KMP_TEST_THEN_ADD4_32(p)                                               \
   __sync_fetch_and_add((volatile kmp_int32 *)(p), 4)
 #define KMP_TEST_THEN_ADD4_ACQ32(p)                                            \
   __sync_fetch_and_add((volatile kmp_int32 *)(p), 4)
+#if defined(KMP_ARCH_MIPS)
+#define KMP_TEST_THEN_ADD4_64(p)                                               \
+  __atomic_fetch_add((volatile kmp_int64 *)(p), 4LL, __ATOMIC_SEQ_CST)
+#define KMP_TEST_THEN_ADD4_ACQ64(p)                                            \
+  __atomic_fetch_add((volatile kmp_int64 *)(p), 4LL, __ATOMIC_SEQ_CST)
+#define KMP_TEST_THEN_DEC64(p)                                                 \
+  __atomic_fetch_sub((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
+#define KMP_TEST_THEN_DEC_ACQ64(p)                                             \
+  __atomic_fetch_sub((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
+#else
 #define KMP_TEST_THEN_ADD4_64(p)                                               \
   __sync_fetch_and_add((volatile kmp_int64 *)(p), 4LL)
 #define KMP_TEST_THEN_ADD4_ACQ64(p)                                            \
   __sync_fetch_and_add((volatile kmp_int64 *)(p), 4LL)
-#define KMP_TEST_THEN_DEC32(p)                                                 \
-  __sync_fetch_and_sub((volatile kmp_int32 *)(p), 1)
-#define KMP_TEST_THEN_DEC_ACQ32(p)                                             \
-  __sync_fetch_and_sub((volatile kmp_int32 *)(p), 1)
 #define KMP_TEST_THEN_DEC64(p)                                                 \
   __sync_fetch_and_sub((volatile kmp_int64 *)(p), 1LL)
 #define KMP_TEST_THEN_DEC_ACQ64(p)                                             \
   __sync_fetch_and_sub((volatile kmp_int64 *)(p), 1LL)
+#endif
+#define KMP_TEST_THEN_DEC32(p)                                                 \
+  __sync_fetch_and_sub((volatile kmp_int32 *)(p), 1)
+#define KMP_TEST_THEN_DEC_ACQ32(p)                                             \
+  __sync_fetch_and_sub((volatile kmp_int32 *)(p), 1)
 #define KMP_TEST_THEN_ADD8(p, v)                                               \
   __sync_fetch_and_add((volatile kmp_int8 *)(p), (kmp_int8)(v))
 #define KMP_TEST_THEN_ADD32(p, v)                                              \
   __sync_fetch_and_add((volatile kmp_int32 *)(p), (kmp_int32)(v))
+#if defined(KMP_ARCH_MIPS)
+#define KMP_TEST_THEN_ADD64(p, v)                                              \
+  __atomic_fetch_add((volatile kmp_uint64 *)(p), (kmp_uint64)(v),              \
+                     __ATOMIC_SEQ_CST)
+#else
 #define KMP_TEST_THEN_ADD64(p, v)                                              \
   __sync_fetch_and_add((volatile kmp_int64 *)(p), (kmp_int64)(v))
+#endif
 
 #define KMP_TEST_THEN_OR8(p, v)                                                \
   __sync_fetch_and_or((volatile kmp_int8 *)(p), (kmp_int8)(v))
 #define KMP_TEST_THEN_AND8(p, v)                                               \
   __sync_fetch_and_and((volatile kmp_int8 *)(p), (kmp_int8)(v))
 #define KMP_TEST_THEN_OR32(p, v)                                               \
   __sync_fetch_and_or((volatile kmp_uint32 *)(p), (kmp_uint32)(v))
 #define KMP_TEST_THEN_AND32(p, v)                                              \
   __sync_fetch_and_and((volatile kmp_uint32 *)(p), (kmp_uint32)(v))
+#if defined(KMP_ARCH_MIPS)
+#define KMP_TEST_THEN_OR64(p, v)                                               \
+  __atomic_fetch_or((volatile kmp_uint64 *)(p), (kmp_uint64)(v),               \
+                    __ATOMIC_SEQ_CST)
+#define KMP_TEST_THEN_AND64(p, v)                                              \
+  __atomic_fetch_and((volatile kmp_uint64 *)(p), (kmp_uint64)(v),              \
+                     __ATOMIC_SEQ_CST)
+#else
 #define KMP_TEST_THEN_OR64(p, v)                                               \
   __sync_fetch_and_or((volatile kmp_uint64 *)(p), (kmp_uint64)(v))
 #define KMP_TEST_THEN_AND64(p, v)                                              \
   __sync_fetch_and_and((volatile kmp_uint64 *)(p), (kmp_uint64)(v))
+#endif
 
 #define KMP_COMPARE_AND_STORE_ACQ8(p, cv, sv)                                  \
   __sync_bool_compare_and_swap((volatile kmp_uint8 *)(p), (kmp_uint8)(cv),     \
                                (kmp_uint8)(sv))
 #define KMP_COMPARE_AND_STORE_REL8(p, cv, sv)                                  \
   __sync_bool_compare_and_swap((volatile kmp_uint8 *)(p), (kmp_uint8)(cv),     \
                                (kmp_uint8)(sv))
 #define KMP_COMPARE_AND_STORE_ACQ16(p, cv, sv)                                 \
   __sync_bool_compare_and_swap((volatile kmp_uint16 *)(p), (kmp_uint16)(cv),   \
                                (kmp_uint16)(sv))
 #define KMP_COMPARE_AND_STORE_REL16(p, cv, sv)                                 \
   __sync_bool_compare_and_swap((volatile kmp_uint16 *)(p), (kmp_uint16)(cv),   \
                                (kmp_uint16)(sv))
 #define KMP_COMPARE_AND_STORE_ACQ32(p, cv, sv)                                 \
   __sync_bool_compare_and_swap((volatile kmp_uint32 *)(p), (kmp_uint32)(cv),   \
                                (kmp_uint32)(sv))
 #define KMP_COMPARE_AND_STORE_REL32(p, cv, sv)                                 \
   __sync_bool_compare_and_swap((volatile kmp_uint32 *)(p), (kmp_uint32)(cv),   \
                                (kmp_uint32)(sv))
-#define KMP_COMPARE_AND_STORE_ACQ64(p, cv, sv)                                 \
-  __sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),   \
-                               (kmp_uint64)(sv))
-#define KMP_COMPARE_AND_STORE_REL64(p, cv, sv)                                 \
-  __sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),   \
-                               (kmp_uint64)(sv))
 #define KMP_COMPARE_AND_STORE_PTR(p, cv, sv)                                   \
   __sync_bool_compare_and_swap((void *volatile *)(p), (void *)(cv),            \
                                (void *)(sv))
 
 #define KMP_COMPARE_AND_STORE_RET8(p, cv, sv)                                  \
   __sync_val_compare_and_swap((volatile kmp_uint8 *)(p), (kmp_uint8)(cv),      \
                               (kmp_uint8)(sv))
 #define KMP_COMPARE_AND_STORE_RET16(p, cv, sv)                                 \
   __sync_val_compare_and_swap((volatile kmp_uint16 *)(p), (kmp_uint16)(cv),    \
                               (kmp_uint16)(sv))
 #define KMP_COMPARE_AND_STORE_RET32(p, cv, sv)                                 \
   __sync_val_compare_and_swap((volatile kmp_uint32 *)(p), (kmp_uint32)(cv),    \
                               (kmp_uint32)(sv))
+#if defined(KMP_ARCH_MIPS)
+static inline bool mips_sync_bool_compare_and_swap(
+  volatile kmp_uint64 *p, kmp_uint64 cv, kmp_uint64 sv) {
+  return __atomic_compare_exchange(p, &cv, &sv, false, __ATOMIC_SEQ_CST,
+                                                       __ATOMIC_SEQ_CST);
+}
+static inline bool mips_sync_val_compare_and_swap(
+  volatile kmp_uint64 *p, kmp_uint64 cv, kmp_uint64 sv) {
+  __atomic_compare_exchange(p, &cv, &sv, false, __ATOMIC_SEQ_CST,
+                                                __ATOMIC_SEQ_CST);
+  return cv;
+}
+#define KMP_COMPARE_AND_STORE_ACQ64(p, cv, sv)                                 \
+  mips_sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),\
+                               (kmp_uint64)(sv))
+#define KMP_COMPARE_AND_STORE_REL64(p, cv, sv)                                 \
+  mips_sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),\
+                               (kmp_uint64)(sv))
+#define KMP_COMPARE_AND_STORE_RET64(p, cv, sv)                                 \
+  mips_sync_val_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv), \
+                              (kmp_uint64)(sv))
+#else
+#define KMP_COMPARE_AND_STORE_ACQ64(p, cv, sv)                                 \
+  __sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),   \
+                               (kmp_uint64)(sv))
+#define KMP_COMPARE_AND_STORE_REL64(p, cv, sv)                                 \
+  __sync_bool_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),   \
+                               (kmp_uint64)(sv))
 #define KMP_COMPARE_AND_STORE_RET64(p, cv, sv)                                 \
   __sync_val_compare_and_swap((volatile kmp_uint64 *)(p), (kmp_uint64)(cv),    \
                               (kmp_uint64)(sv))
+#endif
 
 #define KMP_XCHG_FIXED8(p, v)                                                  \
   __sync_lock_test_and_set((volatile kmp_uint8 *)(p), (kmp_uint8)(v))
 #define KMP_XCHG_FIXED16(p, v)                                                 \
   __sync_lock_test_and_set((volatile kmp_uint16 *)(p), (kmp_uint16)(v))
 #define KMP_XCHG_FIXED32(p, v)                                                 \
   __sync_lock_test_and_set((volatile kmp_uint32 *)(p), (kmp_uint32)(v))
 #define KMP_XCHG_FIXED64(p, v)                                                 \