PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time

[adalava]

Summary

FreeBSD on powerpc 32 bit is moving to LLVM (actually, it's now using LLVM[1] since Dec 26. I enabled atomic.c on this platform and found it was emiting calls to make decisions about lock/lock free at runtime.

We know in advance 8 byte (64 bits) operations need lock and there's no plans to create something like a "libatomic", so this patch.

As FreeBSD13-CURRENT ships this patch, I would like your inputs. Thanks!

[1] PowerPC clang announcement - https://lists.freebsd.org/pipermail/freebsd-ppc/2019-December/011042.html

[nemanjai]

LGTM.

clang/lib/AST/ExprConstant.cpp

11211

Please add to this comment that this may need to be restricted to specific operating systems in the future as some (perhaps AIX) might provide libatomic.

[MaskRay]

I am still confused why you need the special rule for __atomic_is_lock_free (GCC/clang) and __c11_atomic_is_lock_free (clang).

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L7300-L7314 GCC __atomic_is_lock_free expands to either 1 or a library call to __atomic_is_lock_free, never 0. How did FreeBSD get around libatomic when it was still using GCC? In clang, We can probably extend the semantics of __c11_atomic_is_lock_free and use clang::TargetInfo::getMaxAtomicPromoteWidth, but I'll make more research in this area.

Can you be elaborate how do __atomic_is_lock_free/__c11_atomic_is_lock_free pull in libatomic.{a,so} dependency on FreeBSD powerpc?

[adalava]

In D71600#1816160, @MaskRay wrote:

I am still confused why you need the special rule for __atomic_is_lock_free (GCC/clang) and __c11_atomic_is_lock_free (clang).

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L7300-L7314 GCC __atomic_is_lock_free expands to either 1 or a library call to __atomic_is_lock_free, never 0. How did FreeBSD get around libatomic when it was still using GCC? In clang, We can probably extend the semantics of __c11_atomic_is_lock_free and use clang::TargetInfo::getMaxAtomicPromoteWidth, but I'll make more research in this area.

Can you be elaborate how do __atomic_is_lock_free/__c11_atomic_is_lock_free pull in libatomic.{a,so} dependency on FreeBSD powerpc?

On FreeBSD 11.x/12.x (GCC4), libatomic can be installed by user through "gcc9" ports package. The library is not shipped with the base system and it's not required to compile the base system (world + kernel) since nothing depends on 64 bit atomics an external call so __atomic_is_lock_free is never made.

The problem appeared when I switched to clang in a new ISO, this added an undesired external libatomic dependency to build FreeBSD base sources (world): clang was emitting a call to external __atomic_is_lock_free implementation when building clang itself from base sources. (user would need to install gcc9 to be able to build FreeBSD base source using clang, it's not acceptable.

[jfb]

In D71600#1816834, @adalava wrote:

In D71600#1816160, @MaskRay wrote:

I am still confused why you need the special rule for __atomic_is_lock_free (GCC/clang) and __c11_atomic_is_lock_free (clang).

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L7300-L7314 GCC __atomic_is_lock_free expands to either 1 or a library call to __atomic_is_lock_free, never 0. How did FreeBSD get around libatomic when it was still using GCC? In clang, We can probably extend the semantics of __c11_atomic_is_lock_free and use clang::TargetInfo::getMaxAtomicPromoteWidth, but I'll make more research in this area.

Can you be elaborate how do __atomic_is_lock_free/__c11_atomic_is_lock_free pull in libatomic.{a,so} dependency on FreeBSD powerpc?

On FreeBSD 11.x/12.x (GCC4), libatomic can be installed by user through "gcc9" ports package. The library is not shipped with the base system and it's not required to compile the base system (world + kernel) since nothing depends on 64 bit atomics an external call so __atomic_is_lock_free is never made.

The problem appeared when I switched to clang in a new ISO, this added an undesired external libatomic dependency to build FreeBSD base sources (world): clang was emitting a call to external __atomic_is_lock_free implementation when building clang itself from base sources. (user would need to install gcc9 to be able to build FreeBSD base source using clang, it's not acceptable.

I'm curious about this: what part of clang makes it emit a call to is_lock_free? clang itself probably shouldn't be doing this, so maybe we should fix that issue as well.

[adalava]

Updated comment

[dim]

clang/lib/AST/ExprConstant.cpp

11209

s/emiting/emitting/

[adalava]

fix typo found by @dim

[efriedma]

For the clang change, we should do something like D72579, not explicitly check for a specific target in target-independent code.

For compiler-rt, are you really disabling COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN? Are you sure you understand the implications of that?

I'm also curious: what part of clang is calling __atomic_is_lock_free? I can't find any code in LLVM that calls it.

[adalava]

In D71600#1867135, @efriedma wrote:

For the clang change, we should do something like D72579, not explicitly check for a specific target in target-independent code.

right, I'll retest everything using D72579.

For compiler-rt, are you really disabling COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN? Are you sure you understand the implications of that?

I didn't understood "disable COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN", it's not intentional.
If it's the change around atomic.c:131, what I expect is make IS_LOCK_FREE_8 return false. I don't want it to make to __c11_atomic_is_lock_free(8) as it generates code that should be linked with a libatomic at run time.

I'm also curious: what part of clang is calling __atomic_is_lock_free? I can't find any code in LLVM that calls it.

hm, I'm afraid I was not clear in this. When generating FreeBSD images, the libc cross-compiled by unpatched clang gets an entry to external call to __c11_atomic_is_lock_free(). Then, in the resulting system (new sysroot) I get this problem (libatomic dependency) when trying to build clang itself.
While testing with D72579 I'll try reproduce it again and will post more info here since I don't have the build logs anymore (I investigated it ~6 months ago).

[efriedma]

In D71600#1867320, @adalava wrote:

For compiler-rt, are you really disabling COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN? Are you sure you understand the implications of that?

I didn't understood "disable COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN", it's not intentional.
If it's the change around atomic.c:131, what I expect is make IS_LOCK_FREE_8 return false. I don't want it to make to __c11_atomic_is_lock_free(8) as it generates code that should be linked with a libatomic at run time.

On master, atomic.c is not built by default. It's only built if you explicitly request it with something like the CMake flag -DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=Off . If you're not doing that, any change to atomic.c has no effect.

[adalava]

In D71600#1868284, @efriedma wrote:

On master, atomic.c is not built by default. It's only built if you explicitly request it with something like the CMake flag -DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=Off . If you're not doing that, any change to atomic.c has no effect.

I see. In FreeBSD src/base, LLVM isn't using the standard cmake scripts, so we explicitly add atomic.c to the build, we don't set this flag (I did this: https://reviews.freebsd.org/D22549#change-3IJWKySKm5fg). However it's a good point, we must check if COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN is used for something else, and we should probably add the flag when building ports packages (devel/llvm*). Thank you for pointing that.

[efriedma]

atomic.c is disabled by default for a good reason: it doesn't work correctly in general. In particular, if an atomic variable is shared across two shared libraries, the two libraries will use different locks, and therefore the operation won't be atomic.

It might make sense to provide a shared library compiler-rt-atomic or something like that for targets that want it, but someone would have to implement it. (Or maybe on ELF targets we could make the symbols unique by messing with the linkage... but that might cause other issues.)

clang/lib/AST/ExprConstant.cpp

if (Size.isPowerOfTwo()) {
         if (!PointeeType->isIncompleteType() &&
             Info.Ctx.getTypeAlignInChars(PointeeType) >= Size) {
           // OK, we will inline operations on this object.
           return Success(1, E);
         }
       }
     }
 
+    // Avoid emitting call for runtime decision on PowerPC 32-bit
+    // The lock free possibilities on this platform are covered by the lines 
+    // above and we know in advance other cases require lock.
+    // This may need to be restricted to specific operating systems in the future,
+    // as some (perhaps AIX) might provide libatomic and prefer to use it instead
+    if (Info.Ctx.getTargetInfo().getTriple().getArch() == llvm::Triple::ppc) {
+        return Success(0, E);
+    }
+
     return BuiltinOp == Builtin::BI__atomic_always_lock_free ?
         Success(0, E) : Error(E);
   }
   case Builtin::BIomp_is_initial_device:
     // We can decide statically which value the runtime would return if called.
     return Success(Info.getLangOpts().OpenMPIsDevice ? 0 : 1, E);
   case Builtin::BI__builtin_add_overflow:
   case Builtin::BI__builtin_sub_overflow:

compiler-rt/lib/builtins/atomic.c

static __inline Lock *lock_for_pointer(void *ptr) {
   // Now use the high(er) set of bits to perturb the hash, so that we don't
   // get collisions from atomic fields in a single object
   hash >>= 16;
   hash ^= low;
   // Return a pointer to the word to use
   return locks + (hash & SPINLOCK_MASK);
 }
 
-/// Macros for determining whether a size is lock free.  Clang can not yet
-/// codegen __atomic_is_lock_free(16), so for now we assume 16-byte values are
-/// not lock free.
+/// Macros for determining whether a size is lock free.
 #define IS_LOCK_FREE_1 __c11_atomic_is_lock_free(1)
 #define IS_LOCK_FREE_2 __c11_atomic_is_lock_free(2)
 #define IS_LOCK_FREE_4 __c11_atomic_is_lock_free(4)
+
+/// 32 bit PowerPC doesn't support 8-byte lock_free atomics
+#if !defined(__powerpc64__) && defined(__powerpc__)
+#define IS_LOCK_FREE_8 0
+#else
 #define IS_LOCK_FREE_8 __c11_atomic_is_lock_free(8)
+#endif
+
+/// Clang can not yet codegen __atomic_is_lock_free(16), so for now we assume
+/// 16-byte values are not lock free.
 #define IS_LOCK_FREE_16 0
 
 /// Macro that calls the compiler-generated lock-free versions of functions
 /// when they exist.
 #define LOCK_FREE_CASES()                                                      \
   do {                                                                         \
     switch (size) {                                                            \
     case 1:                                                                    \