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

[Philip Reames via llvm-commits]

reames added a comment.

I would strongly prefer to see the two NFC changes separated and submitted separately.  (Assuming you're reasonable sure they are NFC, that doesn't need further review.)  Having everything in the same patch makes it harder to spot semantic changes in the diff.

Mostly minor comments follow.

I have to admit, I don't really understand the distinction between atomic_*, sync_*, and the buildins even after reading the documentation.  Do you have any suggestions for further reading?


================
Comment at: include/llvm/CodeGen/RuntimeLibcalls.h:500
@@ -432,3 +499,3 @@
   /// UNKNOWN_LIBCALL if there is none.
-  Libcall getATOMIC(unsigned Opc, MVT VT);
+  Libcall getSYNC(unsigned Opc, MVT VT);
 }
----------------
Separable NFC change?

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:115
@@ +114,3 @@
+  unsigned Align = LI->getAlignment();
+  if (Align == 0)
+    return DL.getABITypeAlignment(LI->getType());
----------------
Alignment can't be zero for atomic instructions.  Make this an assert.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:123
@@ +122,3 @@
+  unsigned Align = SI->getAlignment();
+  if (Align == 0)
+    return DL.getABITypeAlignment(SI->getValueOperand()->getType());
----------------
Same as previous

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:129
@@ +128,3 @@
+unsigned getAtomicOpAlign(AtomicRMWInst *RMWI) {
+  // TODO: This instruction has no alignment attribute, but unlike the
+  // default alignment for load/store, the default here is to assume
----------------
This seems like an inconsistency we should fix.  Could you file a bug for this so that we don't forget?

Also, this function probably makes sense to promote to getAlignment on the AtomicRMWInst class itself.  

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:941
@@ +940,3 @@
+// memory_order_* value required by the __atomic_* libcalls.
+static int libcallAtomicModel(AtomicOrdering AO) {
+  switch (AO) {
----------------
Use an enum for the memory_order_* values.  Also, don't we have that somewhere already?


http://reviews.llvm.org/D18200