<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
I don't have much of substance to add, but I will say that I like
the proposal (I too prefer Alternative A). When adding C11 atomic
support for hexagon, I found it surprising that support for the
__sync_* was implemented completely differently than the C11
atomics.<br>
<br>
<div class="moz-cite-prefix">On 1/27/2016 1:55 PM, James Knight via
llvm-dev wrote:<br>
</div>
<blockquote
cite="mid:03A17DD1-3693-47AF-B3DC-D3C7A9C6A6D3@google.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">Right now, the
atomics implementation in clang is a bit of a mess. It has three
basically completely distinct code-paths:</div>
<ol class="">
<li style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">There's the
legacy __sync_* builtins, which clang lowers directly to
atomic IR instructions. Then, the llvm atomic IR instructions
themselves can sometimes emit libcalls to __sync_* library
functions (which are basically undocumented, and users are
often responsible for implementing themselves if they need
it).</li>
<li style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">There's the new
__atomic_* builtins, which clang will, depending on size and
alignment and target, lower either to a libcall to a "<a
moz-do-not-send="true"
href="https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary"
class=""><span style="color: rgb(18, 85, 204);
-webkit-text-stroke-color: rgb(18, 85, 204);" class="">standardized-by-GCC</span></a>"
__atomic_* library function (implemented by libatomic), or, to
the atomic IR instructions. (Also covered by the same code is
the __c11_atomic_* builtins, which are almost-identical
clang-specific aliases of the __atomic_* builtins.)</li>
<li style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">There's the C11
"_Atomic" type modifier and some OpenMP stuff, which clang
lowers into atomic IR or libcalls to __atomic_* library
functions, via almost completely separate code from #2. (Note:
doesn't apply to C++ std::atomic<>, which gets
implemented with the builtins instead). Beyond just a lot of
duplicated logic in the code, the _Atomic impl is kinda
broken: sometimes it emits llvm IR instructions directly
(letting things fall back to __sync_* calls), and sometimes it
checks if it should emit a libcall first (falling back to
__atomic_* calls). That's pretty bad behavior.</li>
</ol>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">I'd like to make a
proposal for cleaning this mess up.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">BTW, as a
sidenote...</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">One thing that's
important to remember: at least on the C/C++ level, if you
support lock-free atomic-ops for a given size/alignment, <i
class="">ALL</i> the atomic ops for that size/alignment must
be lock-free. This property is usually quite easy, because
you'll have either LL/SC or CAS instructions, which can be used
to implement any other operation. <span
style="-webkit-text-stroke-width: initial;" class="">However,
many older CPU models support atomic load, store, and exchange
operations, but are missing any way to do an atomic
compare-and-swap. This is true for at least ARMv5, SPARCv8,
and Intel 80386. When your minimum CPU is set to such a cpu
model, all atomic operations must be done via libcall -- it's
not acceptable for atomic_store to emit an atomic "store"
instruction, but atomic_fetch_add to require a libcall which
gets implemented with a lock. If that were to happen, then
atomic_fetch_add could not actually be made atomic versus a
simultaneous atomic_store.</span></div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class=""><br class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">So anyhow, there's
basically two paths I think we could take for cleanup. I like
"Alternative A" below better, but would be interested to hear if
others have reasons to think the other would be preferable for
some reason.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class=""><i class="">Both</i>
alternatives I've suggested will have the effect that the
__sync_* builtins in clang will now lower to __atomic_* function
calls on platforms without inline atomics (or for unsupported
sizes), and C11 atomics will stop being schizophrenic. Clang
will cease to ever emit a call to a __sync_* function from any
of its builtins or language support. <b class="">This could
theoretically cause an incompatibility on some target. </b></div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">However, I think
it ought to be pretty safe: I can't imagine anyone who will use
an upgraded compiler has __sync_* functions implemented for
their platform, but doesn't have the __atomic_* library
functions implemented. The __atomic_* builtins (which already
use those library calls) are in widespread use, notably, both in
libstdc++ and libc++, as well as in a lot of third-party code.
IMO it's worth having that potential incompatibility, in order
to simplify the situation for users (only one set of atomic
library calls to worry about), and to have a single code-path
for atomics in the compiler.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class=""><b class="">Alternative
A</b>: Move all atomic libcall emission into LLVM</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">In this
alternative, LLVM will be responsible for lowering all atomic
operations (to inline code or libcalls as appropriate) for all
three code-paths listed at the beginning. Clang will emit no
libcalls for atomic operations itself.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">A1) In LLVM: when
lowering "store atomic", "load atomic", "atomicrmw", and
"cmpxchg" instructions that aren't supported by the target, emit
libcalls to the new __atomic_* functions, (rather than the
current behavior of calling the legacy __sync_* functions.)</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">Additionally, I'd
like to move the decision to emit a libcall into
AtomicExpandPass, and remove the ability to use Expand in ISel
to create a libcall for ISD::ATOMIC_*. Putting the decision
there allows querying the target, up front, for its supported
sizes, before any other lowering decisions (either other parts
of AtomicExpandPass and in ISel). When choosing whether to
inline or libcall, only the size and alignment of the operation
should be considered, and not the operation itself, or the type.
This will ensure that the "all or none" property is adhered to.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">(Q: what about the
implementation of
__atomic_is_lock_free/__atomic_always_lock_free in clang? The
clang frontend can't query target information from the llvm
backend, can it? Is there some other way to expose the
information of what sizes are supported by a backend so that the
clang builtins -- the latter of which must be a C++ constant
expression -- can also use it? Or...just continue duplicating
the info in both places, as is done today...)</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">A2) In clang,
start removing the code that knows how to do optimized library
call lowering for atomics -- always emit llvm atomic ops. Except
for one case: clang will still need to emit library calls itself
for data not aligned naturally for its size. The LLVM atomic
instructions currently will not handle unaligned data, but
unaligned data is allowed for the four "slab of memory" builtins
(__atomic_load, __atomic_store, __atomic_compare_exchange, and
__atomic_exchange).</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class=""><span
style="-webkit-text-stroke-width: initial;" class="">A3) In
LLVM, add "align" attributes to cmpxchg and atomicrmw, and
allow specifying "align" values for "load atomic" and "store
atomic" (where the attribute currently exists but cannot be
used). LLVM will then be able to lower misaligned accesses to
library calls. In clang, get rid of special case for directly
emitting libcalls for misaligned atomics, and lower to llvm
instructions there, as well.</span></div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><span
style="-webkit-text-stroke-width: initial;" class="">A4)
Hopefully, now, the three codepaths in clang will be
sufficiently the same (as they're all now just creating llvm
atomic instructions) that they can be trivially merged, or
else they are so trivial that the remaining duplication is not
worthwhile to merge.</span></div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class="">
<div class=""><br class="">
</div>
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class="">
<div style="-webkit-text-stroke-width: initial; margin: 0px;
line-height: normal; min-height: 14px;" class=""><br class="">
</div>
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class=""><b class="">Alternative
B</b>: Move all libcall emission for atomics into Clang.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">In this
alternative, LLVM will <i class="">never</i> emit atomic
libcalls from the atomic IR instructions. If the operation
requested is not possible on a given target, that is an error.
So, the cleanups here are to get clang to stop emitting LLVM IR
atomics that cannot be lowered without libcalls.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">B1) In Clang: have
the legacy __sync_* builtins become essentially aliases for the
__atomic_* builtins (thus they will emit calls to __atomic_*
library functions when applicable).</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">B2) In Clang: Have
the C11 _Atomic type support and openmp stuff call into the
__atomic builtins' code to do their dirty work, instead of
having its own separate implementation.</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial; min-height: 14px;" class=""><br
class="">
</div>
<div style="margin: 0px; line-height: normal; color: rgb(35, 35,
35); -webkit-text-stroke-color: rgb(35, 35, 35);
-webkit-text-stroke-width: initial;" class="">B3) After those
two changes, I believe clang will only ever emit atomic IR
instructions when they can be lowered. So then, in LLVM: get rid
of the fallback to libcalls to __sync_* from the atomic IR
instructions. If an atomic IR operation is requested and not
implementable lock-free, it will be an error, with no fallback.</div>
<div class=""><br class="">
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
</pre>
</body>
</html>