Date: Sat, 31 Aug 2013 13:30:02 +0200 From: Ed Schouten <ed@80386.nl> To: David Chisnall <theraven@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r255092 - in head: lib/libcompiler_rt sys/arm/arm Message-ID: <CAJOYFBB63WuWXRCTtj4VbN1G3WBaQ9P6uhWwLDqWJxFNwjztug@mail.gmail.com> In-Reply-To: <201308310850.r7V8ojQX022383@svn.freebsd.org> References: <201308310850.r7V8ojQX022383@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2013/8/31 David Chisnall <theraven@freebsd.org>: > Reviewed by: ed Just for the record: though I did review this patch, but did not agree with the approach taken. Though I have to confess that due all the case distinctions this code isn't the cleanest out there, the #pragmas/__strong_references make it even less readable. In my opinion, this should have been fixed as follows: 1. Fix LLVM/Clang. Never ever let LLVM/Clang emit calls to __sync_*. You can easily implement the __sync_* interface on top of __atomic_*. What baffles me, is that the calls to __sync_* are emitted by LLVM (SelectionDAGLegalize::ExpandAtomic), while Clang is responsible for emitting the __atomic_* calls (CodeGenFunction::EmitAtomicExpr). All of this should have been pushed down into LLVM in the first place. It is currently hard to implement your own programming language on top of LLVM that is capable of doing atomic operations the same way C11/C++11 does them. You either have to use the __sync_* intrinsics or duplicate all the code that emits the libcalls. I've noticed that due to this separation of doing __sync_* in LLVM and __atomic_* in Clang, the behaviour of Clang can sometimes be incredibly counter-intuitive. For example, both LLVM and Clang need a similar piece of code to determine whether hardware atomics are available. I've noticed that if this logic is not identical, Clang does some really weird things. If you call __atomic_* functions and Clang thinks we have hardware atomics, but LLVM thinks we do not, we may end up emitting calls to __sync_* instead. Furthermore, the duplication of code between EmitAtomicExpr, EmitAtomicStore and EmitAtomicLoad leads to the problem that EmitAtomicExpr properly emits power-of-two-sized calls for most primitive datatypes, while EmitAtomicStore and EmitAtomicLoad do not. 2. Fix the consumers. Right now there are only two pieces of code in our tree (libc++ and libcxxrt) that emit calls to __sync_* unconditionally. All the other code either uses <machine/atomic.h> or <stdatomic.h>. These pieces of code could have been ported to use C11 atomics with a similar amount of effort. For libcxxrt you could even argue that this should have done in the first place, because it already used __atomic_* calls in certain source files. Example patch for libcxxrt: http://80386.nl/pub/libcxxrt.txt -- Ed Schouten <ed@80386.nl>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJOYFBB63WuWXRCTtj4VbN1G3WBaQ9P6uhWwLDqWJxFNwjztug>