Page MenuHomeFreeBSD

[PowerPC] enable atomic.c in compiler_rt and makes clang do not emit a call to an external __atomic_is_lock_free
Needs ReviewPublic

Authored by alfredo.junior_eldorado.org.br on Mon, Nov 25, 6:19 PM.

Details

Summary

Enables atomic.c in compiler_rt and forces clang to not emit a symbol calling to an external __atomic_is_lock_free.
At compiling time, if clang can't decide if atomic operation can be lock free, it emits a call to an external __atomic_is_lock_free, so decision can be made at runtime. 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.

  • I'm not sure if the approach in LLVM is acceptable, please let me know your thoughts
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 27886
Build 26057: arc lint + arc unit

Event Timeline

jhibbits added inline comments.Mon, Nov 25, 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.Tue, Nov 26, 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

alfredo.junior_eldorado.org.br marked 3 inline comments as done.Tue, Nov 26, 4:12 PM
luporl added inline comments.Fri, Nov 29, 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.Sun, Dec 1, 3:29 AM
contrib/compiler-rt/lib/builtins/atomic.c
126

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

alfredo.junior_eldorado.org.br marked 4 inline comments as done.

updated after comments from reviewers

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

jhibbits added inline comments.Thu, Dec 5, 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().