Details

Reviewers

bdragon
luporl
jhibbits
dim
emaste

Commits

Summary

Enables atomic.c in compiler_rt and forces clang to not emit a call for runtime decision about lock/lock_free.
At compiling time, if clang can't decide if atomic operation can be lock free, it emits calls to external functions like __atomic_is_lock_free,
__c11_atomic_is_lock_free and __atomic_always_lock_free, postponing decision to a runtime check.
According to LLVM code documentation, the mechanism exists due to differences between x86_64 processors that can't be decided at runtime.

On PowerPC/PowerPCSPE (32 bits), we already know in advance it can't be lock free, so we force the decision at compile time and avoid having to implement it in an external library.

This patch was made after 32 bit users testing the PowePC32 bit ISO reported llvm could not be compiled with in-base llvm due to __atomic_load8 not implemented.

Test Plan

Build 32 bit sysroot and test on powerpc64 machine using chroot (I don't have a truly 32 bit PowerPC machine at the moment)

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 27780
Build 25968: arc lint + arc unit

Event Timeline

alfredo created this revision.Nov 25 2019, 6:19 PM

Harbormaster completed remote builds in B27764: Diff 64860.Nov 25 2019, 6:19 PM

alfredo edited the summary of this revision. (Show Details)Nov 25 2019, 6:22 PM

alfredo added reviewers: bdragon, luporl, jhibbits, dim.

alfredo edited the summary of this revision. (Show Details)Nov 25 2019, 6:27 PM

alfredo added a reviewer: emaste.

jhibbits added inline comments.Nov 25 2019, 6:37 PM

contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9901	We'll be using the ppc triple Arch for powerpcspe.
lib/libcompiler_rt/Makefile.inc
209	We use the same MACHINE_CPUARCH for all powerpc targets

jhibbits added inline comments.Nov 26 2019, 1:27 AM

lib/libcompiler_rt/Makefile.inc
209	You probably want MACHINE_ARCH.

Updated with comments, added a compiler check to make it "commitable" before flagday and retested

Harbormaster completed remote builds in B27780: Diff 64890.Nov 26 2019, 4:11 PM

alfredo marked 3 inline comments as done.Nov 26 2019, 4:12 PM

luporl added inline comments.Nov 29 2019, 2:15 PM

contrib/compiler-rt/lib/builtins/atomic.c
55	Why did you need to change the include order of machine/atomic.h and sys/types.h?
127	Is calling `__c11_atomic_is_lock_free(0)` really the right change here? To achieve the same goal, IS_LOCK_FREE_16 is defined to 0 instead.
contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9908	It is better to avoid introducing a new line here.

jhibbits added inline comments.Dec 1 2019, 3:29 AM

contrib/compiler-rt/lib/builtins/atomic.c
126	No need for defined(powerpcspe), we just use powerpc for powerpcspe target.

updated after comments from reviewers

Harbormaster completed remote builds in B27886: Diff 65122.Dec 2 2019, 1:08 PM

alfredo added inline comments.Dec 2 2019, 1:08 PM

contrib/compiler-rt/lib/builtins/atomic.c
55	"machine/atomic.h" uses types like "uint32_t" that are only defined in "sys/types.h", so it should come first

alfredo added a project: PowerPC.Dec 3 2019, 10:04 PM

jhibbits added inline comments.Dec 5 2019, 6:00 PM

contrib/compiler-rt/lib/builtins/atomic.c
55	I think in accordance with style(9), machine/ includes should come after all sys/ includes.
contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899	This looks redundant. Right below it returns Success(0, E) if the BuiltinOp is __atomic_always_lock_free().

alfredo marked 2 inline comments as done.Dec 12 2019, 8:06 PM

alfredo added inline comments.

contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899	I fact I would to make it avoid emitting external call for "__ atomic_is_lock_free" and "__ c11_atomic_is_lock_free" as well, if ppc32 bit.

jhibbits added inline comments.Dec 13 2019, 5:31 PM

contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899	The comment should reflect that, then. The comment states you just want to avoid __atomic_always_lock_free(), but you want to avoid all 3 on powerpc.