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

[Tim Northover via llvm-commits]

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).

Tim.


================
Comment at: docs/Atomics.rst:483
@@ +482,3 @@
+"standardized" by GCC, and is described below. (See also `GCC's documentation
+<https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary>`_)
+
----------------
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?


http://reviews.llvm.org/D18200