This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by adalava on Dec 17 2019, 4:48 AM.

Details

Reviewers
jfb
dim
xingxue
MaskRay
nemanjai
efriedma
Group Reviewers
Restricted Project
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

Event Timeline

adalava created this revision.Dec 17 2019, 4:48 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 8 others. · View Herald Transcript
adalava added a reviewer: dim.Dec 17 2019, 4:53 AM
adalava added a reviewer: Restricted Project.Jan 9 2020, 11:43 AM
adalava edited the summary of this revision. (Show Details)Jan 9 2020, 1:21 PM
adalava added a reviewer: MaskRay.
nemanjai accepted this revision.Jan 12 2020, 10:13 AM

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.

This revision is now accepted and ready to land.Jan 12 2020, 10:13 AM
MaskRay added a comment.EditedJan 12 2020, 3:23 PM

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?

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 added a comment.Jan 13 2020, 10:14 AM

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 this revision to Diff 239847.Jan 23 2020, 4:00 AM

Updated comment

adalava marked an inline comment as done.Jan 23 2020, 4:00 AM
dim added inline comments.Jan 24 2020, 12:46 PM
clang/lib/AST/ExprConstant.cpp
11209

s/emiting/emitting/

adalava updated this revision to Diff 243520.Feb 10 2020, 5:27 AM

fix typo found by @dim

adalava marked an inline comment as done.Feb 10 2020, 5:28 AM
efriedma requested changes to this revision.Feb 10 2020, 7:37 AM

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.

This revision now requires changes to proceed.Feb 10 2020, 7:37 AM

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

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.

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.

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