[PATCH] Refactor AtomicExpandPass and add a generic isAtomic() method to Instruction

[Robin Morisset]

Patch coming just afterwards, I just answer to a few of the inline comments.

================
Comment at: include/llvm/Target/TargetLowering.h:996
@@ +995,3 @@
+  /// IR-level AtomicExpand pass into a loop using either CmpXchg, or
+  /// LoadLinked/StoreConditional
+  virtual bool shouldExpandAtomicRMWInIR(AtomicRMWInst *RMWI) const {
----------------
jfb wrote:
> Document when CmpXchg or LL/SC are used.
This comment is a mistake on my part, currently LL/SC is always used, support for using CmpXchg is coming in the next patch.

================
Comment at: include/llvm/Target/TargetLowering.h:1000
@@ -987,2 +999,3 @@
+  }
 
   //===--------------------------------------------------------------------===//
----------------
jfb wrote:
> Why no `shouldExpandAtomicCmpXchgInIR`?
Because they should always be expanded when the target supports ll/sc and never when it supports CAS natively. I do not know of any target offering both ll/sc and cas.

The current code assumes that the target has LL/SC, this assumption is removed in another patch that I am hoping to send soon, that makes the X86 backend use this pass. This other patch will add a hasLoadLinkedStoreConditional() target hook which will both decide whether to expand CmpXchgs, and how to expand RMWs.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:91
@@ -90,3 +91,1 @@
-    else
-      llvm_unreachable("Unknown atomic instruction");
   }
----------------
jfb wrote:
> You should probably keep this `else` clause, in case `isAtomic` goes out of sync with this code (just have a do-nothing clause for `Fence`).
Because these conditions also test the shouldExpand*, I cannot leave the llvm_unreachable. Instead I will add an assert.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11065
@@ +11064,3 @@
+// things go wrong. Cortex M doesn't have ldrexd/strexd though, so don't emit
+// anything for those.
+bool ARMTargetLowering::shouldExpandAtomicStoreInIR(StoreInst *SI) const {
----------------
jfb wrote:
> I'm not sure that's quite correct on CortexA: Thumb only has `ldrex`/`strex` on ARMv7, and ARM has them on ARMv6K and ARMv7.
> 
> A further enhancement (doesn't affect correctness): processors can also use `ldrd`/`strd` atomically when `HaveLPAE` is true. On ARM's own processors this requires A15 (I'm not sure about other vendors).
I merely kept the current behaviour and explanation. Is it ok to keep it as-is for now, and look at improving this in a later patch?

http://reviews.llvm.org/D5035