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

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a reviewer: Hahnfeld.
sdardis added a comment.

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.



================
Comment at: runtime/src/kmp_os.h:492
+#if defined(KMP_ARCH_MIPS)
+#define KMP_TEST_THEN_INC64(p)                                                 \
+  __atomic_fetch_add((volatile kmp_int64 *)(p), 1LL, __ATOMIC_SEQ_CST)
----------------
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.


https://reviews.llvm.org/D45691