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

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
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>`_)
+
----------------
t.p.northover wrote:
> Case typo on `LIbrary`?
The strange case actually is the correct URL. Probably a Wiki link thing.

================
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
----------------
t.p.northover wrote:
> 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!
I believe that's not true: ArmV6 had ldrex/strex, but its "Thumb1" mode did not. It seems to have been introduced with "Thumb2" in ARMv6T2 and ARMv7 and later.

The dependent CL adds a new function in ARMSubtarget.h:
  bool hasLdrex() const {
    return HasV6Ops && (!InThumbMode || HasV8MBaselineOps);
  }

(effectively the same logic was previously in ARMSubtarget::enableAtomicExpand)

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:1152-1153
@@ +1151,4 @@
+  if (UseSizedLibcall) {
+    switch (Size) {
+    case 1:
+      RTLibType = Libcalls[1];
----------------
t.p.northover wrote:
> Perhaps `RTLibType = Libcalls[Log2_32(Size) + 1]`?
I dunno, that seems harder to understand to me. Really I think it'd be best as a switch, just with 1/3rd the number of lines in it:
    switch (Size) {
    case 1: RTLibType = Libcalls[1]; break;
    case 2: RTLibType = Libcalls[2]; break;
    case 4: RTLibType = Libcalls[3]; break;
    case 8: RTLibType = Libcalls[4]; break;
    case 16: RTLibType = Libcalls[5]; break;
    }

But, llvm's clang-format style doesn't use
AllowShortCaseLabelsOnASingleLine: true

Maybe it should.

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


http://reviews.llvm.org/D18200