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

[JF Bastien]

================
Comment at: include/llvm/IR/Instruction.h:338
@@ +337,3 @@
+  /// AtomicOrdering of unordered or higher. Currently, this can only be true
+  /// for LoadInst, StoreInst, AtomicRMWInst, AtomicCmpXchgInst, Fence.
+  ///
----------------
Comments tend to go out of sync with the code, so I wouldn't state anything here on which instructions can be atomic.

================
Comment at: include/llvm/Target/TargetLowering.h:981
@@ +980,3 @@
+  /// Returns true if the given (atomic) store should be expanded by the
+  /// IR-level AtomicExpand pass into an "atomic xchg" which ignores its input,
+  virtual bool shouldExpandAtomicStoreInIR(StoreInst *SI) const {
----------------
Comma at the end?

================
Comment at: include/llvm/Target/TargetLowering.h:983
@@ -982,3 +983,1 @@
-  /// same way as "atomic xchg" operations which ignore their output if needed.
-  virtual bool shouldExpandAtomicInIR(Instruction *Inst) const {
     return false;
----------------
You'll probably need to send a PSA to the mailing list to explain the change you're making, and the impact on any out-of-tree implementation of this class (removing one `virtual` method, and adding some).

================
Comment at: include/llvm/Target/TargetLowering.h:989
@@ +988,3 @@
+  /// nothing (if hasLoadLinkedStoreConditional() is false), or into a
+  /// load-linked instruction (through emitLoadLinked())
+  virtual bool shouldExpandAtomicLoadInIR(LoadInst *LI) const {
----------------
Period at the end (same for other 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 {
----------------
Document when CmpXchg or LL/SC are used.

================
Comment at: include/llvm/Target/TargetLowering.h:1000
@@ -987,2 +999,3 @@
+  }
 
   //===--------------------------------------------------------------------===//
----------------
Why no `shouldExpandAtomicCmpXchgInIR`?

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:91
@@ -90,3 +91,1 @@
-    else
-      llvm_unreachable("Unknown atomic instruction");
   }
----------------
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`).

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:373
@@ -374,2 +372,3 @@
   CI->eraseFromParent();
+
   return true;
----------------
Drop this line change.

================
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 {
----------------
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).

http://reviews.llvm.org/D5035