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

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
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!");
+
----------------
jfb wrote:
> 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?
Yeah, you're right. Actually, I'm going to just remove this change entirely. You get an assertion failure:
  llc: llvm/include/llvm/ADT/StringRef.h:73: llvm::StringRef::StringRef(const char *): Assertion `Str && "StringRef cannot be built from a NULL argument"' failed.

anyways, without it. With a stack trace pointing you at the problem.

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

================
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);
----------------
jfb wrote:
> What happens now when ThreadModel is Single? Is that handled elsewhere?
Yeah, this is handled in ARMTargetMachine.cpp, in addIRPasses: if ThreadMode == Single, it adds the LowerAtomic pass, which changes all atomic ops to non-atomic ops, and deletes all fences. That's actually purely dead code at this point. Committed separately.

================
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);
----------------
jfb wrote:
> The comment confuses me: Thumb1 and pre-v6 never get to the else clause, right? The fence is for everyone else?
Thumb1 and pre-v6 do get in here. Then, an IR fence instruction is created, so that it will get expanded into the libcall. CPUs with actual fence instructions use special intrisincs instead, in order to make the particularly appropriate kinds of fence instruction needed here.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12029
@@ +12028,3 @@
+// libcalls, expansion to __atomic_* routines would've already
+// happened).
+//
----------------
jfb wrote:
> Seems better to just change the name of the hasLdrex function instead of explaining it here :-)
Well, hasLdrex is actually a good name. I'm trying to explain why I'm *using* it here. But apparently failed -- I've reworded, hopefully more clearly.

================
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:
----------------
jfb wrote:
> Missing CHECKs.
Oops!

================
Comment at: test/CodeGen/ARM/atomic-load-store.ll:91
@@ +90,3 @@
+; THUMBONE-NEXT: ___sync_synchronize
+; THUMBONE-NEXT: ___sync_synchronize
+; THUMBONE-NEXT: strb
----------------
jfb wrote:
> Ugh we should eliminate redundant libcalls elsewhere...
Yes, it might be nice to have a redundant fence elimination pass. You could also sometimes remove a fence which is redundant with a neighboring atomic instruction.

E.g.: "atomic_store seq_cst; atomic_fence seq_cst" could potentially eliminate the fence on some (all existing?) targets.

But that's a whole other project.


http://reviews.llvm.org/D18201