Page MenuHomePhabricator

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

Authored by mgorny on Jan 1 2017, 2:57 PM.

Details

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.

Event Timeline

mgorny updated this revision to Diff 82791.Jan 1 2017, 2:57 PM
mgorny retitled this revision from to [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin.
mgorny updated this object.
mgorny added a subscriber: cfe-commits.
hfinkel accepted this revision.Jan 8 2017, 2:18 PM
hfinkel edited edge metadata.

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

This revision is now accepted and ready to land.Jan 8 2017, 2:18 PM
This revision was automatically updated to reflect the committed changes.
dim added a subscriber: dim.Feb 3 2017, 10:26 AM

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.

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 added a comment.Feb 3 2017, 1:45 PM
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 added a comment.Feb 3 2017, 2:20 PM
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?

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 added a comment.Feb 4 2017, 1:12 AM
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 added a comment.Feb 4 2017, 3:23 AM

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

mgorny reopened this revision.Mar 23 2017, 10:25 AM

Reopening since it has been reverted.

This revision is now accepted and ready to land.Mar 23 2017, 10:25 AM

@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 ?

Herald added a project: Restricted Project. · View Herald Transcript
jfb added a comment.Mar 19 2019, 1:14 PM

Why was it reverted?

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 added a comment.Mar 19 2019, 1:55 PM

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...

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).

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;
 };

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

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:34 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
jyknight reopened this revision.Oct 7 2019, 7:37 AM

The close was due to phabricator problem, reopening.

This revision is now accepted and ready to land.Oct 7 2019, 7:37 AM