[PATCH] D18201: Switch over targets to use AtomicExpandPass, and clean up target atomics code.

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: include/llvm/Target/TargetLowering.h:165
@@ -164,1 +164,3 @@
 
+  /// Allow lowering into __sync_* libcalls.
+  void initSyncLibcalls();
----------------
Would be nice to document what it does otherwise.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:1436-1438
@@ -1435,1 +1435,5 @@
 SDValue SelectionDAG::getExternalSymbol(const char *Sym, EVT VT) {
+  if (!Sym)
+    report_fatal_error(
+        "Attempted to use null symbol in SelectionDAG::getExternalSymbol!");
+
----------------
t.p.northover wrote:
> Isn't this an assertion-level error? It doesn't seem to be actionable by the user in any reasonable way.
Yes, should probably be a separate change anyways?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:10124
@@ -10122,3 +10123,3 @@
   unsigned Size = SI->getValueOperand()->getType()->getPrimitiveSizeInBits();
   return Size == 128;
 }
----------------
Is the assumption here that Size > 128 never gets to this code? Could that be asserted?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:10135
@@ -10134,3 +10134,3 @@
 
-// For the real atomic operations, we have ldxr/stxr up to 128 bits,
+// Expand RWM operations to ldrex/strex instructions.
 TargetLowering::AtomicExpansionKind
----------------
"RMW"

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:844
@@ -843,3 @@
-  InsertFencesForAtomic = false;
-  if (TM.Options.ThreadModel == ThreadModel::Single)
-    setOperationAction(ISD::ATOMIC_FENCE,   MVT::Other, Expand);
----------------
What happens now when ThreadModel is Single? Is that handled elsewhere?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11969
@@ +11968,3 @@
+      // mode just use a libcall to __sync_synchronize. So, just emit
+      // a fence instruction.
+      return Builder.CreateFence(AtomicOrdering::SequentiallyConsistent);
----------------
The comment confuses me: Thumb1 and pre-v6 never get to the else clause, right? The fence is for everyone else?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12029
@@ +12028,3 @@
+// libcalls, expansion to __atomic_* routines would've already
+// happened).
+//
----------------
Seems better to just change the name of the hasLdrex function instead of explaining it here :-)

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12044
@@ -12046,3 +12043,3 @@
   unsigned Size = SI->getValueOperand()->getType()->getPrimitiveSizeInBits();
-  return (Size == 64) && !Subtarget->isMClass();
+  return Size == 64;
 }
----------------
Same here, size can't be > 64? Assert?

================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:131
@@ -130,1 +130,3 @@
 
+  // This backend handles atomics ops up to 64bits.
+  setMaxAtomicSizeSupported(64);
----------------
This comment seems kind of obvious from the code.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:57
@@ -53,1 +56,3 @@
+  else
+    setMaxAtomicSizeSupported(32);
   // Set up the register classes.
----------------
This is correct for wasm (whereas I'm not certain for some of the other ISAs).

================
Comment at: test/CodeGen/ARM/atomic-cmpxchg.ll:103
@@ +102,3 @@
+
+define zeroext i1 @test_cmpxchg_res_i64(i64* %addr, i64 %desired, i64 zeroext %new) {
+entry:
----------------
Missing CHECKs.

================
Comment at: test/CodeGen/ARM/atomic-load-store.ll:91
@@ +90,3 @@
+; THUMBONE-NEXT: ___sync_synchronize
+; THUMBONE-NEXT: ___sync_synchronize
+; THUMBONE-NEXT: strb
----------------
Ugh we should eliminate redundant libcalls elsewhere...

================
Comment at: test/CodeGen/ARM/atomic-op.ll:263
@@ -259,3 +262,3 @@
 
-; CHECK: func4
+; CHECK-LABEL: func4
 ; This function should not need to use callee-saved registers.
----------------
Colon after func4:


http://reviews.llvm.org/D18201