[Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

[rprichard]

Summary

Correct the logic used to set ATOMIC_*_LOCK_FREE preprocessor macros not
to rely on the ABI alignment of types. Instead, just assume all those
types are aligned correctly by default since clang uses safe alignment
for _Atomic types even if the underlying types are aligned to a lower
boundary by default.

For example, the long long and double types on x86 are aligned to
32-bit boundary by default. However, _Atomic long long and `_Atomic
double` are aligned to 64-bit boundary, therefore satisfying
the requirements of lock-free atomic operations.

This fixes PR #19355 by correcting the value of
__GCC_ATOMIC_LLONG_LOCK_FREE on x86, and therefore also fixing
the assumption made in libc++ tests. This also fixes PR #30581 by
applying a consistent logic between the functions used to implement
both interfaces.

[hfinkel]

LGTM. This seems consistent with what GCC does. On x86 it also sets __GCC_ATOMIC_LLONG_LOCK_FREE to 2.

[dim]

In D28213#639397, @hfinkel wrote:

LGTM. This seems consistent with what GCC does. On x86 it also sets __GCC_ATOMIC_LLONG_LOCK_FREE to 2.

Hmm, unfortunately on i386-freebsd, it does not:

$ gcc6 -v
Using built-in specs.
COLLECT_GCC=gcc6
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc6/gcc/i386-portbld-freebsd12.0/6.3.0/lto-wrapper
Target: i386-portbld-freebsd12.0
Configured with: /wrkdirs/usr/ports/lang/gcc6/work/gcc-6.3.0/configure --disable-multilib --disable-bootstrap --disable-nls --enable-gnu-indirect-function --libdir=/usr/local/lib/gcc6 --libexecdir=/usr/local/libexec/gcc6 --program-suffix=6 --with-as=/usr/local/bin/as --with-gmp=/usr/local --with-gxx-include-dir=/usr/local/lib/gcc6/include/c++/ --with-ld=/usr/local/bin/ld --with-pkgversion='FreeBSD Ports Collection' --with-system-zlib --disable-libgcj --enable-languages=c,c++,objc,fortran --prefix=/usr/local --localstatedir=/var --mandir=/usr/local/man --infodir=/usr/local/info/gcc6 --build=i386-portbld-freebsd12.0
Thread model: posix
gcc version 6.3.0 (FreeBSD Ports Collection)

$ gcc6 -dM -E -x c /dev/null | grep __GCC_ATOMIC_LLONG_LOCK_FREE
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1

$ clang -v
FreeBSD clang version 4.0.0 (branches/release_40 293807) (based on LLVM 4.0.0)
Target: i386-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin

$ clang -dM -E -x c /dev/null | grep __GCC_ATOMIC_LLONG_LOCK_FREE
#define __GCC_ATOMIC_LLONG_LOCK_FREE 2

I don't think FreeBSD has lock-free 64 bit atomic operations on 32-bit x86. IIRC we already had some trouble before with clang emitting libcalls to __atomic_fetch_add_8 and friends, which then lead to linking errors.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216745, where this is now occurring with boost.

[mgorny]

In D28213#665967, @dim wrote:

I don't think FreeBSD has lock-free 64 bit atomic operations on 32-bit x86. IIRC we already had some trouble before with clang emitting libcalls to __atomic_fetch_add_8 and friends, which then lead to linking errors.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216745, where this is now occurring with boost.

Could you try to figure out what's the cause for that discrepancy? What's the value of __atomic_always_lock_free(sizeof(long long), 0) for gcc and clang?

[dim]

In D28213#666196, @mgorny wrote:

In D28213#665967, @dim wrote:

I don't think FreeBSD has lock-free 64 bit atomic operations on 32-bit x86. IIRC we already had some trouble before with clang emitting libcalls to __atomic_fetch_add_8 and friends, which then lead to linking errors.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216745, where this is now occurring with boost.

Could you try to figure out what's the cause for that discrepancy?

I think the discrepancy is that many 32-bit Linuxes target at least i686 by default, and then you have CMPXCHG8B most of the time (apparently not always, but nobody seems to care):

$ uname -srm
Linux 3.16.0-4-686-pae i686
$ gcc -dM -E -x c /dev/null | grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 2

However, 32-bit FreeBSD targets i486 by default:

$ uname -srm
FreeBSD 12.0-CURRENT i386
$ gcc -dM -E -x c /dev/null | grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1
$ clang-3.9.1 -dM -E -x c /dev/null | grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1
$ clang-4.0.0 -dM -E -x c /dev/null | grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 2

Note that even on 32-bit Linux you get 1 for __GCC_ATOMIC_LLONG_LOCK_FREE, if you target i486:

$ uname -srm
Linux 3.16.0-4-686-pae i686
$ gcc -march=i486 -dM -E -x c /dev/null | grep LLONG_LOCK
#define __GCC_ATOMIC_LLONG_LOCK_FREE 1

What's the value of __atomic_always_lock_free(sizeof(long long), 0) for gcc and clang?

For gcc, it is always 0, for clang (I tested 3.4.1 through 4.0.0) it is always 1. Maybe that was always incorrect on 32-bit FreeBSD, then?

[dim]

In D28213#666269, @dim wrote:

In D28213#666196, @mgorny wrote:

...

What's the value of __atomic_always_lock_free(sizeof(long long), 0) for gcc and clang?

For gcc, it is always 0, for clang (I tested 3.4.1 through 4.0.0) it is always 1. Maybe that was always incorrect on 32-bit FreeBSD, then?

Hmm, I just noticed the following rather disappointing comment in tools/clang/lib/Basic/Targets.cpp:

X86_32TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
    : X86TargetInfo(Triple, Opts) {
  [...]
  // x86-32 has atomics up to 8 bytes
  // FIXME: Check that we actually have cmpxchg8b before setting
  // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;

So this can never have worked properly for e.g. i486 and i586...

[mgorny]

In D28213#666269, @dim wrote:

What's the value of __atomic_always_lock_free(sizeof(long long), 0) for gcc and clang?

For gcc, it is always 0, for clang (I tested 3.4.1 through 4.0.0) it is always 1. Maybe that was always incorrect on 32-bit FreeBSD, then?

That's what I suspected. Yeah, looks like clang always made the wrong assumption and possibly someone just swept it under the carpet.

In D28213#666352, @dim wrote:

Hmm, I just noticed the following rather disappointing comment in tools/clang/lib/Basic/Targets.cpp:

X86_32TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
    : X86TargetInfo(Triple, Opts) {
  [...]
  // x86-32 has atomics up to 8 bytes
  // FIXME: Check that we actually have cmpxchg8b before setting
  // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;

So this can never have worked properly for e.g. i486 and i586...

Yep, looks like it. I see that the triple is passed there, so I guess making that conditional to >=i586-* should be relatively easy. However, I don't know how to make it properly respect -march.

[dim]

Created https://llvm.org/bugs/show_bug.cgi?id=31864 to continue this on the bug tracker.

[mgorny]

Reopening since it has been reverted.

[erichkeane]

@mgorny @hfinkel @eli.friedman @jyknight @dim
Is there any chance we can get this in any time soon? It fixes a couple of header issues that we've noticed. Others added, since it appears this dependent on https://reviews.llvm.org/D29542 ?

[jfb]

Why was it reverted?

[efriedma]

It looks like it was reverted because it was breaking i386 BSD, where __GCC_ATOMIC_LLONG_LOCK_FREE is in fact supposed to be "1" (because cmpxchg8b isn't always available).

[jfb]

In D28213#1435485, @efriedma wrote:

It looks like it was reverted because it was breaking i386 BSD, where __GCC_ATOMIC_LLONG_LOCK_FREE is in fact supposed to be "1" (because cmpxchg8b isn't always available).

Do we care about cases where it *might* be available? i.e. can we say it's never available instead?

We're playing with ABI here, it's not great, but at the same time we're bending over backwards for pretty old CPUs... all of that to get bad code generation...

[efriedma]

Do we care about cases where it *might* be available? i.e. can we say it's never available instead?

That doesn't really help here... the fundamental issue is that getMaxAtomicInlineWidth() is wrong (or at least, was wrong at the time this was initially merged; I haven't checked whether it was fixed since).

[craig.topper]

It's still wrong. I think this might fix it?

--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -346,9 +346,8 @@ public:
          (1 << TargetInfo::LongDouble));

     // x86-32 has atomics up to 8 bytes
-    // FIXME: Check that we actually have cmpxchg8b before setting
-    // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
-    MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+    MaxAtomicPromoteWidth = 64;
+    MaxAtomicInlineWidth = 32;
   }

   BuiltinVaListKind getBuiltinVaListKind() const override {
@@ -384,6 +383,11 @@ public:
     return X86TargetInfo::validateOperandSize(Constraint, Size);
   }

+  void setMaxAtomicWidth() override {
+    if (CPU >= CK_i586)
+      MaxAtomicInlineWidth = 64;
+  }
+
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 };

[efriedma]

It's kind of awkward to use ">=" on a CPU enum, but yes, that's the right idea.

[craig.topper]

In D28213#1435542, @efriedma wrote:

It's kind of awkward to use ">=" on a CPU enum, but yes, that's the right idea.

I agree, but we do the same thing on the "__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8" define in X86.cpp.

Applying that diff does cause 3 tests to fail. I'll try to work through that.

[craig.topper]

Patch here https://reviews.llvm.org/D59566

[jyknight]

The close was due to phabricator problem, reopening.

[lkail]

Hi, is this patch ready to land?

[rprichard]

@mgorny Do you mind if I commandeer this Phabricator revision (and also D29542, which I would abandon in favor of D59566 which is already merged)? I need this CL to get libc++ tests passing on i686-linux after D119931 was submitted.

I rebased the InitPreprocessor.cpp change and updated two tests (Sema/atomic-ops.c and Preprocessor/init-x86.c). The Preprocessor/cuda-types.cu test is also failing, but I haven't investigated that yet.

Aside: FreeBSD switched its default CPU from i486 to i686 (D83645), but Clang's NetBSD target is still on 486, so that doesn't have cmpxchg8b by default.

[rprichard]

Rebase this revision.

[rprichard]

Stylistic change: keep the -ALIGN32 and -ALIGN64 suffixes in the test.

[rprichard]

@efriedma @craig.topper Could you review this patch (or suggest someone else that could)?

[efriedma]

LGTM

cfe/trunk/lib/Frontend/InitPreprocessor.cpp

static void DefineFastIntType(unsigned TypeWidth, bool IsSigned,
   DefineTypeSize(Prefix + Twine(TypeWidth) + "_MAX__", Ty, TI, Builder);
 
   DefineFmt(Prefix + Twine(TypeWidth), Ty, TI, Builder);
 }
 
 
 /// Get the value the ATOMIC_*_LOCK_FREE macro should have for a type with
 /// the specified properties.
-static const char *getLockFreeValue(unsigned TypeWidth, unsigned TypeAlign,
-                                    unsigned InlineWidth) {
+static const char *getLockFreeValue(unsigned TypeWidth, unsigned InlineWidth) {
   // Fully-aligned, power-of-2 sizes no larger than the inline
   // width will be inlined as lock-free operations.
-  if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
-      TypeWidth <= InlineWidth)
+  // Note: we do not need to check alignment since _Atomic(T) is always
+  // appropriately-aligned in clang.
+  if ((TypeWidth & (TypeWidth - 1)) == 0 && TypeWidth <= InlineWidth)
     return "2"; // "always lock free"
   // We cannot be certain what operations the lib calls might be
   // able to implement as lock-free on future processors.
   return "1"; // "sometimes lock free"
 }
 
 /// \brief Add definitions required for a smooth interaction between
 /// Objective-C++ automated reference counting and libstdc++ (4.2).
if (!LangOpts.MSVCCompat) {
     // FIXME: This is target-dependent.
     Builder.defineMacro("__GCC_ATOMIC_TEST_AND_SET_TRUEVAL", "1");
 
     // Used by libc++ and libstdc++ to implement ATOMIC_<foo>_LOCK_FREE.
     unsigned InlineWidthBits = TI.getMaxAtomicInlineWidth();
 #define DEFINE_LOCK_FREE_MACRO(TYPE, Type) \
     Builder.defineMacro("__GCC_ATOMIC_" #TYPE "_LOCK_FREE", \
                         getLockFreeValue(TI.get##Type##Width(), \
-                                         TI.get##Type##Align(), \
                                          InlineWidthBits));
     DEFINE_LOCK_FREE_MACRO(BOOL, Bool);
     DEFINE_LOCK_FREE_MACRO(CHAR, Char);
     DEFINE_LOCK_FREE_MACRO(CHAR16_T, Char16);
     DEFINE_LOCK_FREE_MACRO(CHAR32_T, Char32);
     DEFINE_LOCK_FREE_MACRO(WCHAR_T, WChar);
     DEFINE_LOCK_FREE_MACRO(SHORT, Short);
     DEFINE_LOCK_FREE_MACRO(INT, Int);
     DEFINE_LOCK_FREE_MACRO(LONG, Long);
     DEFINE_LOCK_FREE_MACRO(LLONG, LongLong);
     Builder.defineMacro("__GCC_ATOMIC_POINTER_LOCK_FREE",
                         getLockFreeValue(TI.getPointerWidth(0),
-                                         TI.getPointerAlign(0),
                                          InlineWidthBits));
 #undef DEFINE_LOCK_FREE_MACRO
   }
 
   if (LangOpts.NoInlineDefine)
     Builder.defineMacro("__NO_INLINE__");
 
   if (unsigned PICLevel = LangOpts.PICLevel) {

cfe/trunk/test/Sema/atomic-ops.c

 // RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11
 
 // Basic parsing/Sema tests for __c11_atomic_*
 
 #include <stdatomic.h>
 
 struct S { char c[3]; };
 
 _Static_assert(__GCC_ATOMIC_BOOL_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_CHAR_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_CHAR16_T_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_CHAR32_T_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_WCHAR_T_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_SHORT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_INT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_LONG_LOCK_FREE == 2, "");
-#ifdef __i386__
-_Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 1, "");
-#else
 _Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 2, "");
-#endif
 _Static_assert(__GCC_ATOMIC_POINTER_LOCK_FREE == 2, "");
 
 _Static_assert(__c11_atomic_is_lock_free(1), "");
 _Static_assert(__c11_atomic_is_lock_free(2), "");
 _Static_assert(__c11_atomic_is_lock_free(3), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(4), "");
 _Static_assert(__c11_atomic_is_lock_free(8), "");
 _Static_assert(__c11_atomic_is_lock_free(16), ""); // expected-error {{not an integral constant expression}}