[PATCH] D18200: Add __atomic_* lowering to AtomicExpandPass.

Tim Northover via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 16 10:56:29 PDT 2016

t.p.northover added a subscriber: t.p.northover.
t.p.northover added a comment.

Very nice! The eventual Clang simplification should be a huge relief and more than make up for this code (not to mention other backends). In general the code looks fine (most of my suggestions are nit-picking over comments).


Comment at: docs/Atomics.rst:483
@@ +482,3 @@
+"standardized" by GCC, and is described below. (See also `GCC's documentation
Case typo on `LIbrary`?

Comment at: docs/Atomics.rst:553-554
@@ +552,4 @@
+on function-call boundaries. For example, MIPS supports the MIPS16 ISA, which is
+has a smaller instruction encoding than the usual MIPS32 ISA. ARM, similarly,
+has the Thumb ISA. In MIPS16 and earlier versions of Thumb, the atomic
+instructions are not encodable. However, those instructions are available via a
I probably wouldn't mention ARM here, since I think ldrex/strex is always available in Thumb mode if it is in ARM. We won't feel neglected, honestly!

Comment at: include/llvm/Target/TargetLowering.h:1055
@@ +1054,3 @@
+  /// Whether the DAG builder should automatically insert fences and
+  /// reduce ordering for this atomic. This should be true for
"atomic lowering" instead of "DAG builder", since we're changing it around anyway?

Comment at: lib/CodeGen/AtomicExpandPass.cpp:1098-1099
@@ +1097,4 @@
+  // the operation (min/max), or there were only size-specialized
+  // libcalls (add/sub/etc) and we needed a generic. So, expand to a
+  // CAS loop instead.
+  if (!Success) {
"So, expand to a CAS libcall instead, via a CAS loop"? I was panicking there for a while until I saw the `expandAtomicCASToLibcall`.

Comment at: lib/CodeGen/AtomicExpandPass.cpp:1152-1153
@@ +1151,4 @@
+  if (UseSizedLibcall) {
+    switch (Size) {
+    case 1:
+      RTLibType = Libcalls[1];
Perhaps `RTLibType = Libcalls[Log2_32(Size) + 1]`?

Comment at: lib/CodeGen/AtomicExpandPass.cpp:1228
@@ +1227,3 @@
+    AllocaCASExpected =
+        AllocaBuilder.CreateAlloca(CASExpected->getType(), nullptr, "");
+    AllocaCASExpected->setAlignment(AllocaAlignment);
The last 2 args are the defaults aren't they?


More information about the llvm-commits mailing list