[libcxx][AIX][PowerPC] Disable workaround for PR31864 on powerpc

[lkail]

Summary

IIUC, this workaround is for x86 to solve inconsistency between clang defined __GCC_ATOMIC_LLONG_LOCK_FREE(or __CLANG_ATOMIC_LLONG_LOCK_FREE) and __atomic_always_lock_free(8, 0), i.e., for x86 cpu >= i586, __GCC_ATOMIC_LLONG_LOCK_FREE equals to 1(sometimes-lockfree), but __atomic_always_lock_free(8, 0) returns true. See https://github.com/llvm/llvm-project/issues/31212.

This looks not an issue for powerpc.

[dim]

Hm @pkubaj do you remember if this is needed for 32-bit PowerPC on FreeBSD? I can't remember...

[pkubaj]

No idea, @adalava or @jhibbits may know.

[adalava]

long long (8-bytes) can be lock free on PPC64, but it's not lock free on PPC32.

Since we know this in advance, clang should not emit a libatomic call for runtime decision for long long on PPC32. The historical discussion is here: https://reviews.freebsd.org/D22549 (code has changed since then).

[ldionne]

libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp

29

It looks like this issue was fixed a long time ago. Do we need a workaround at all anymore? I would suggest removing the workaround entirely and seeing whether our CI passes.

[lkail]

Removed the workaround and trigger libcxx-ci.

[lkail]

Though https://buildkite.com/llvm-project/libcxx-ci/builds/8557 passed all tests, it looks not covering x86. I tried running buildbot locally on my x86 laptop with

--- a/libcxx/cmake/caches/Generic-cxx17.cmake
+++ b/libcxx/cmake/caches/Generic-cxx17.cmake
@@ -1,2 +1,8 @@
 set(LIBCXX_TEST_PARAMS "std=c++17" CACHE STRING "")
 set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
+set(LIBCXX_TARGET_TRIPLE "i686-unknown-linux-gnu" CACHE STRING "")
+set(LIBUNWIND_TARGET_TRIPLE "i686-unknown-linux-gnu" CACHE STRING "")
+set(LIBCXXABI_TARGET_TRIPLE "i686-unknown-linux-gnu" CACHE STRING "")
+set(CMAKE_C_FLAGS "-m32" CACHE STRING "")
+set(CMAKE_CXX_FLAGS "-m32" CACHE STRING "")

It fails with

$ "/opt/LLVM-12.0.1-Linux/bin/clang++" "/home/lkail/repos/llvm-project/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp" "--target=i686-unknown-linux-gnu" "-nostdinc++" "-isystem" "/home/lkail/repos/llvm-project/libcxx-ci/include/c++/v1" "-isystem" "/home/lkail/repos/llvm-project/libcxx-ci/include/c++/v1" "-I" "/home/lkail/repos/llvm-project/libcxx/test/support" "-std=c++17" "-Werror" "-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-user-defined-literals" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-Wno-macro-redefined" "-D_LIBCPP_HAS_THREAD_API_PTHREAD" "-Wno-macro-redefined" "-D_LIBCPP_ABI_VERSION=1" "-lc++experimental" "-nostdlib++" "-L" "/home/lkail/repos/llvm-project/libcxx-ci/lib" "-Wl,-rpath,/home/lkail/repos/llvm-project/libcxx-ci/lib" "-lc++" "-pthread" "-o" "/home/lkail/repos/llvm-project/libcxx-ci/std/atomics/atomics.lockfree/Output/isalwayslockfree.pass.cpp.dir/t.tmp.exe"
# command stderr:
/home/lkail/repos/llvm-project/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp:30:3: error: static_assert failed due to requirement 'std::atomic<long long>::is_always_lock_free == (2 == 1)' ""
  static_assert(std::atomic<long long>::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), "");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/lkail/repos/llvm-project/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp:31:3: error: static_assert failed due to requirement 'std::atomic<unsigned long long>::is_always_lock_free == (2 == 1)' ""
  static_assert(std::atomic<unsigned long long>::is_always_lock_free == (2 == ATOMIC_LLONG_LOCK_FREE), "");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

error: command failed with exit status: 1

There are 32-bit platforms in libcxx-ci, armv7 and armv8, but I'm not an expert of ARM CPUs, I guess these ARM cores should support 8-bytes lock-free atomics on 32-bit platforms, so that they are passing w/o this patch.

[lkail]

long long (8-bytes) can be lock free on PPC64, but it's not lock free on PPC32.

That's true.

Since I don't have freebsd on powerpc at hand, I just check macros defined

~/llvm/dev/build/bin/clang --target=powerpc-ibm-aix-xcoff -dM -E -x c /dev/null | grep LLONG_LOCK
#define __CLANG_ATOMIC_LLONG_LOCK_FREE 1
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1

~/llvm/dev/build/bin/clang --target=powerpc-unknown-freebsd-unknown -dM -E -x c /dev/null | grep LLONG_LOCK
#define __CLANG_ATOMIC_LLONG_LOCK_FREE 1
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1

FreeBSD has same values as AIX, I suppose this test would also fail on powerpc-unknown-freebsd-unknown.

[ldionne]

Sorry, I don't quite understand. Did we fix the underlying issue in Clang (https://github.com/llvm/llvm-project/issues/31212) or did we not?

[lkail]

In D119246#3308055, @ldionne wrote:

Sorry, I don't quite understand. Did we fix the underlying issue in Clang (https://github.com/llvm/llvm-project/issues/31212) or did we not?

It looks not fixed or regressed, clang's output for i686(and also i586) is still inconsistent with gcc's.

gcc -march=i686 -m32 -dM -E -x c /dev/null|grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 2
gcc -v
gcc version 8.5.0 20210514 (Red Hat 8.5.0-4) (GCC)

clang -march=i686 -m32 -dM -E -x c /dev/null|grep LLONG_LOCK
#define __CLANG_ATOMIC_LLONG_LOCK_FREE 1
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1
clang -v
clang version 15.0.0 (git://github.com/llvm/llvm-project.git 1831cbd9d4174a93d4017e510ecf0f840af5f7d6)

The problem here is we expect clang defined __CLANG_ATOMIC_LLONG_LOCK_FREE and __GCC_ATOMIC_LLONG_LOCK_FREE to be 2 for x86 CPU >= i586. This workaround is still required for x86 CPU >= i586.

Hi @craig.topper , you mentioned

This is needed to get the LLONG_LOCK_FREE macro to be 2 on i586 and greater.

in https://reviews.llvm.org/D59566#1437512. Do you have plan currently to get it fixed?

[lkail]

Hi @ldionne , I've opened an issue on github https://github.com/llvm/llvm-project/issues/53840, do you agree we currently keep the workaround and unblock the fix on AIX?

[ldionne]

In D119246#3321815, @lkail wrote:

Hi @ldionne , I've opened an issue on github https://github.com/llvm/llvm-project/issues/53840, do you agree we currently keep the workaround and unblock the fix on AIX?

Sorry for the delay, but I think this can be abandoned now, right? Since https://github.com/llvm/llvm-project/issues/53840 has been fixed and I just approved something to remove this workaround. If so, please abandon to clear up the queue.

[lkail]

Abandon it since https://reviews.llvm.org/D119931 is the alternative.

libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: c++03, c++11, c++14
 
-// Fails for 32-bit builds.
-// UNSUPPORTED: LIBCXX-AIX-FIXME
-
 // <atomic>
 
 // static constexpr bool is_always_lock_free;
 
 #include <atomic>
 #include <cassert>
 
 #include "test_macros.h"
 
 #if !defined(__cpp_lib_atomic_is_always_lock_free)
 # error Feature test macro missing.
 #endif
 
 template <typename T> void checkAlwaysLockFree() {
   if (std::atomic<T>::is_always_lock_free)
     assert(std::atomic<T>().is_lock_free());
 }
 
 // FIXME: This separate test is needed to work around llvm.org/PR31864
 // which causes ATOMIC_LLONG_LOCK_FREE to be defined as '1' in 32-bit builds
 // even though __atomic_always_lock_free returns true for the same type.
 constexpr bool NeedWorkaroundForPR31864 =
-#if defined(__clang__)
+#if defined(__clang__) && !defined(__powerpc__)
 (sizeof(void*) == 4); // Needed on 32 bit builds
 #else
 false;
 #endif
 
 template <bool Disable = NeedWorkaroundForPR31864,
   std::enable_if_t<!Disable>* = nullptr,
   class LLong = long long,