Page MenuHomeFreeBSD

[PowerPC] enable atomic.c in compiler_rt and do not check and forces lock/lock_free decisions in compiled time
ClosedPublic

Authored by alfredo on Nov 25 2019, 6:19 PM.

Details

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

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alfredo added a reviewer: emaste.
contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9901 ↗(On Diff #64860)

We'll be using the ppc triple Arch for powerpcspe.

lib/libcompiler_rt/Makefile.inc
209 ↗(On Diff #64860)

We use the same MACHINE_CPUARCH for all powerpc targets

lib/libcompiler_rt/Makefile.inc
209 ↗(On Diff #64860)

You probably want MACHINE_ARCH.

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

contrib/compiler-rt/lib/builtins/atomic.c
55 ↗(On Diff #64890)

Why did you need to change the include order of machine/atomic.h and sys/types.h?

127 ↗(On Diff #64890)

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 ↗(On Diff #64890)

It is better to avoid introducing a new line here.

contrib/compiler-rt/lib/builtins/atomic.c
126 ↗(On Diff #64890)

No need for defined(powerpcspe), we just use powerpc for powerpcspe target.

alfredo marked 4 inline comments as done.

updated after comments from reviewers

contrib/compiler-rt/lib/builtins/atomic.c
55 ↗(On Diff #64890)

"machine/atomic.h" uses types like "uint32_t" that are only defined in "sys/types.h", so it should come first

contrib/compiler-rt/lib/builtins/atomic.c
55 ↗(On Diff #65122)

I think in accordance with style(9), machine/ includes should come after all sys/ includes.

contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899 ↗(On Diff #65122)

This looks redundant. Right below it returns Success(0, E) if the BuiltinOp is __atomic_always_lock_free().

alfredo added inline comments.
contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899 ↗(On Diff #65122)

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.

contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
9899 ↗(On Diff #65122)

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.

alfredo marked an inline comment as done.
alfredo retitled this revision from [PowerPC] enable atomic.c in compiler_rt and makes clang do not emit a call to an external __atomic_is_lock_free to [PowerPC] enable atomic.c in compiler_rt and do not check and forces lock/lock_free decisions in compiled time.
alfredo edited the summary of this revision. (Show Details)

Removed OS check and fixed comment

This revision is now accepted and ready to land.Dec 13 2019, 7:45 PM