incorrect logic in opal_config_asm.m4 for 128-wide atomic ops

[flang-cavium]

Thank you for taking the time to submit an issue!

Patch is based on OpenMPI 3.1.1 - released tarball.

Compiled and installed from source.

Operating system: Ubuntu 16.04 AArch64 (ARM64).
Compilers:

• GCC 7.3 and 8.1
• clang/LLVM 6.0.1

Problem description:

The logic for discovering and enabling 128-bit wide atomic ops at ./configure time is incorrect. Essentially, 128-wide atomic ops are enabled regardless of whether or not the Target actually supports them.

The file opal_config_asm.m4 contains the following:

if test ! "$enable_cross_cmpset128" = "yes" ; then
### enable 128-wide atomic cmpset
fi

The logical test

if test ! "$enable_cross_cmpset128" = "yes" ; then

is equivalent to

if test "$enable_cross_cmpset128" = "no" ; then

which means, cmpset128 will be enabled even though ./configure was told

--enable-cross-cmpset128=no

Further down, there is another test for $enable_cross_cmpset128:

if test ! "$enable_cross_cmpset128" = "yes" ; then

### enable 128-wide atomic CAS

else

### enable 128-wide atomic CAS

fi

if test ! "$enable_cross_cmpset128" = "yes" ; then

is the equivalent of saying

if test "$enable_cross_cmpset128" = "no" ; then

Again, same logic problem, but this time around making certain 128-wide CAS really gets enable no matter what ./configure says.

Also: trying to attach a patch named opal_config_asm.m4.patch results in the following error:

We don't support that file type. Try again with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP.

I seriously doubt anyone would ever submit a GIF, JPEG, JPG or PNG patch. I had to rename the patch file from opal_config_asm.m4.patch to opal_config_asm.m4.patch.txt.
opal_config_asm.m4.patch.txt

[bosilca]

The current logic is correct, the code you pinpoint to handles both the support for and the cross compiling support for 128 bits atomics. If support for 128 bits atomics exists, it cannot be disabled as it is the safest (and most efficient way) to cope with some ABA issues. The conditions are written in a reversed way, but the logic itself holds.

[jsquyres]

@flang-cavium George is correct. I'll close this issue; please feel free to reply if you have further questions.

And here's why attaching an image is sometimes useful 😄:

[flang-cavium]

Ummm, no.

George is not correct. Actually, George is 100% wrong.

This has nothing to do with cross-compiling.

Two-value logic: "yes" or "no". ! "yes" means "no". Oui? Non?

I have already explained in the bug in detail. Care to actually take a look?

[flang-cavium]

Question: "ABA" -- American Banking Association?

[bosilca]

Actually I did look at your bug description and at your patch and the current code before writing my answer. Your conclusion doesn't stand.

ABA stands for the ABA problem, a multi-threaded synchronization issue that arise when 2 variables must be both atomically updated. 128 bits wide atomic operations provides a simple and efficient solution.

[flang-cavium]

So, in your esteemed opinion, cmpxchg16b should always be enabled, regardless of whether or not the ISA supports it, or whether or not ./configure said --enable-cross-cmpset128=no and --enable-cross-cmpxchg16b=no?

Is this what you are saying here?

What does "128 bits wide atomic operations provides a simple and efficient solution" really mean?

Not all ISA's suport 128-bit wide atomics. Not even all flavors of X86_64 support it.

You still haven't explained what this has to do with cross-compiling.

[flang-cavium]

Not to mention that the ./configure test for 128-wide atomics when compiling with GCC is inherently broken:

int main()
{
  __int128 x = 0, y = 0;
  __atomic_compare_exchange_n (&x, &y, 1, 0, __ATOMIC_RELAXED,
                               __ATOMIC_RELAXED);
  return 0;
}

/usr/local/gcc72/bin/gcc -D_GNU_SOURCE -O2 -std=c99 -mtune=corei7 -mfma -mavx -Wl,-rpath -Wl,/usr/local/gcc72/lib64 int128.c -o int128-gcc
/tmp/ccjdCiyN.o: In function main': int128.c:(.text.startup+0x29): undefined reference to __atomic_compare_exchange_16'
collect2: error: ld returned 1 exit status

So, when compiling with GCC, the test fails at link-time. GCC needs -latomic on link-line, which is absent in the ./configure test.

This ends up making cmpxchg16b and the 128-wide atomics always disabled when compiling with GCC. Problem is, cmpxchg16b with GCC ends up being disabled for all the wrong reasons. 128-wide atomics with GCC will end up disabled even for those X86_64 micro-arch'es that actually support 128-wide atomics and cmpxchg16b.

[bosilca]

enable_cross_cmpset128 is not the typical enable/disable argument, it only needs one single value to hint configure to do a special check for the cross-compile case. The reason is that when not cross-compiling one can run a test during configure to validate the needed support, including looking for the existence of 128-bits atomic operations, and the flags or libraries such support requires.

And yes, I am saying that 128 bits atomics should always be used when available. The only way to turn it off is to completely disable all builtin atomics via --disable-builtin-atomics.

Anyway, you pinpointed a real problem: OMPI fails to detect support for 16 bits atomics on newer compilers that provide atomic support via the atomic library. Patch underway.

[flang-cavium]

We're getting there.

I am saying that 128 bits atomics should always be used when available.

Exactly. 128-wide atomics are not available on every single ISA.

What you are advocating here is an all-or-nothing proposition.

Either we have 128-wide atomics, in which case we can use atomics, or we don't, in which case we can't use atomics.

This is wrong.

32-wide and 64-bit atomics are available on ISA's that do not have 128-wide atomics. For example, ARM64. Which means we should be able to use 32-bit and 64-bit atomics even when the 128-wide atomics aren't available.

Which is precisely what ./configure pretends it does, but lies about it:

--enable-cross-cmpset128
--enable-cross-cmpxchg16b

If I pass "no" to either or both these ./configure arguments, the 128-wide atomics should be disabled. But they are not disabled. They are always enabled, although I am not even cross-compiling.

Which means ./configure should - at a minimum - test whether or not I am cross-compiling or not. And if I am not cross-compiling, they should be disabled unless I am explicitly enabling them with

--enable-cross-cmpset128=yes
--enable-cross-cmpxchg16b=yes

And if you would have taken the time to test the patch, instead of arbitrarily discarding it wtihout testing, you would have noticed that the patch does exactly what you want it to do, and cures the problem of having 128-wide atomics always enabled, even for the ISA's that do not support them.

[bosilca]

You have deeply misunderstood or misinterpreted every single comment I made. As I said before I read and test your patch, and it does not work.

[flang-cavium]

No, I don't think I misunderstood anything.

I have tested my patch as well. And it works. I seriously doubt you tested it.

[bosilca]

Let me break it down for you:

1. Your initial analysis is misleading. You claim that due to the enable_cross_cmpset128 check we enable 128 bits atomics on all branches. Wrong. We enable checking for 128 bits atomics support on all branches, but this checking could result in turning them off.
2. You raised a valid point about not using -latomic, but your patch doesn't even addresses this. Moreover, adding -latomic is not required in all cases, in some cases using the -mcx16 flag is enough.
3. You have certainly successfully tested your patch on ARM64 but on a x86_64 using your patch and one of the compilers you mentioned (gcc 7.3) the 128 bits atomics are completely disabled (OPAL_HAVE_SYNC_BUILTIN_CSWAP_INT128 and OPAL_HAVE_GCC_BUILTIN_CSWAP_INT128 unset). Clearly incorrect because the __sync natively supports 128 atomics in this environment.
4. It turns out there is a bug in opal_config_asm.m4 when handling __atomic_always_lock_free. If your patch would have correctly enabled 128 bits atomics you would have noticed it.

Please take a look at #5546 to see if it addresses your issue.