This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Remove workaround for CMPXCHG
AbandonedPublic

Authored by Hahnfeld on Aug 7 2019, 6:38 AM.

Details

Summary

GCC complains that 'cmp' in OP_CMPXCHG_WORKAROUND is not initialized.
I think this is wrong because it is written to through the pointer
in 'vvv'. However, the workaround is dated 2007 (we're in 2019!) and
was supposed to be fixed in version 11.0 of the compiler, so I think
the easiest solution is to just remove the old code.

I looked at the generated assembly of __kmpc_atomic_cmplx4_add / _sub
and it looks the same as __kmpc_atomic_float8_add except that cmplx4 are
two single precision values while float8 is one double precision value.
I also verified the result of a small Fortran program that atomically
adds to a shared complex(kind=4) variable in a parallel region.
The code in __kmpc_atomic_cmplx4_mul / _div also seems to load the
volatile variable which was the problem cited in the comment above
the workaround. All in all, I think this bug is no more.

Diff Detail

Event Timeline

Hahnfeld created this revision.Aug 7 2019, 6:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
Hahnfeld updated this revision to Diff 214558.Aug 11 2019, 4:15 AM
Hahnfeld edited the summary of this revision. (Show Details)

Remove compiler check for -Wno-uninitialized.

jlpeyton requested changes to this revision.Aug 12 2019, 11:34 AM

Unfortunately, the Windows atomic code will not compile with this change. See in kmp_atomic.h the different atomic types for Windows. The complaints are regarding assignment operations for the complex types.

../../src/kmp_atomic.cpp(1325): error: class "__kmp_cmplx32_t" has no suitable assignment operator
  ATOMIC_CMPXCHG(cmplx4, add, kmp_cmplx32, 64, +, 8c, 7,
  ^

You can simulate the build errors on Linux by applying:

diff --git a/src/kmp_atomic.h b/src/kmp_atomic.h
index a42421a..b45a474 100644
--- a/src/kmp_atomic.h
+++ b/src/kmp_atomic.h
@@ -55,7 +55,7 @@
 //                  to use typedef'ed types on win.
 // Condition for WIN64 was modified in anticipation of 10.1 build compiler.

-#if defined(__cplusplus) && (KMP_OS_WINDOWS)
+#if defined(__cplusplus) && (KMP_OS_WINDOWS || KMP_OS_LINUX)
 // create shortcuts for c99 complex types

 // Visual Studio cannot have function parameters that have the
This revision now requires changes to proceed.Aug 12 2019, 11:34 AM

Unfortunately, the Windows atomic code will not compile with this change. See in kmp_atomic.h the different atomic types for Windows. The complaints are regarding assignment operations for the complex types.

../../src/kmp_atomic.cpp(1325): error: class "__kmp_cmplx32_t" has no suitable assignment operator
  ATOMIC_CMPXCHG(cmplx4, add, kmp_cmplx32, 64, +, 8c, 7,
  ^

Oh wow, that code is ugly, and GCC loudly complains that a C function returns a C++ struct. Is it still true that "Intel compiler does not support _Complex datatype on win." or could we possibly get rid of __kmp_cmplx64_t and __kmp_cmplx32_t?

The problem is that OP_CMPXCHG casts lhs of type TYPE * to TYPE volatile *. This works for primitive data types, but not for structs because it can't use the implemented assignment operator.

Hahnfeld abandoned this revision.Nov 16 2019, 9:43 AM

I won't continue work on this.