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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Sorry for the delay!

In https://reviews.llvm.org/D45691#1210989, @jlpeyton wrote:

> In https://reviews.llvm.org/D45691#1210590, @Hahnfeld wrote:
>
> > Ok, it looks like https://reviews.llvm.org/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?



================
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)
----------------
sdardis wrote:
> 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.
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`.


https://reviews.llvm.org/D45691