Canonicalize all "idempotent" atomicrmw ops

[reames]

Summary

For "idempotent" atomicrmw instructions which we can't simply turn into load, canonicalize the operation and constant. This reduces the matching needed elsewhere in the optimizer, but doesn't directly impact codegen.

For any architecture where OR/Zero is not a good default choice, you can extend the AtomicExpand lowerIdempotentRMWIntoFencedLoad mechanism. I reviewed X86 to make sure this works well, haven't audited other backends.

[jfb]

lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

65 ↗

(On Diff #186872)

I don't think you want to canonicalize volatile accesses this way.

test/Transforms/InstCombine/atomicrmw.ll

92 ↗

(On Diff #186872)

typo

[reames]

Address review comments

[reames]

lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

65 ↗

(On Diff #186872)

Why? It's not changing the memory op implied. I can easy restrict, but why?

[jfb]

lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

65 ↗

(On Diff #186872)

It's volatile, we just shouldn't touch it. The user asked for something silly, let's do something silly.

llvm/trunk/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

 }
 }
 
 
 Instruction *InstCombiner::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
   if (!isIdempotentRMW(RMWI))
     return nullptr;
 
-  // TODO: Canonicalize the operation for an idempotent operation we can't
-  // convert into a simple load.
-
-  // Volatile RMWs perform a load and a store, we cannot replace
-  // this by just a load.
+  // Volatile RMWs perform a load and a store, we cannot replace this by just a
+  // load. We chose not to canonicalize out of general paranoia about user
+  // expectations around volatile. 
   if (RMWI.isVolatile())
     return nullptr;
 
+  // We chose to canonicalize all idempotent operations to an single
+  // operation code and constant.  This makes it easier for the rest of the
+  // optimizer to match easily.  The choice of or w/zero is arbitrary.
+  if (RMWI.getType()->isIntegerTy() &&
+      RMWI.getOperation() != AtomicRMWInst::Or) {
+    RMWI.setOperation(AtomicRMWInst::Or);
+    RMWI.setOperand(1, ConstantInt::get(RMWI.getType(), 0));
+    return &RMWI;
+  }
+
   // Check if the required ordering is compatible with an atomic load.
   AtomicOrdering Ordering = RMWI.getOrdering();
   assert(Ordering != AtomicOrdering::NotAtomic &&
          Ordering != AtomicOrdering::Unordered &&
          "AtomicRMWs don't make sense with Unordered or NotAtomic");
   if (Ordering != AtomicOrdering::Acquire &&
       Ordering != AtomicOrdering::Monotonic)
     return nullptr;
   
   LoadInst *Load = new LoadInst(RMWI.getType(), RMWI.getPointerOperand());
   Load->setAtomic(Ordering, RMWI.getSyncScopeID());
   Load->setAlignment(DL.getABITypeAlignment(RMWI.getType()));
   return Load;
 }

llvm/trunk/test/Transforms/InstCombine/atomicrmw.ll

 ; CHECK-LABEL: atomic_syncscope
 ; CHECK-NEXT: %res = load atomic i16, i16* %addr syncscope("some_syncscope") acquire, align 2
 ; CHECK-NEXT: ret i16 %res
 define i16 @atomic_syncscope(i16* %addr) {
   %res = atomicrmw or i16* %addr, i16 0 syncscope("some_syncscope") acquire
   ret i16 %res
 }
 
-; Don't transform seq_cst ordering.
 ; By eliminating the store part of the atomicrmw, we would get rid of the
-; release semantic, which is incorrect.
-; CHECK-LABEL: atomic_or_zero_seq_cst
+; release semantic, which is incorrect.  We can canonicalize the operation.
+; CHECK-LABEL: atomic_seq_cst
 ; CHECK-NEXT: %res = atomicrmw or i16* %addr, i16 0 seq_cst
 ; CHECK-NEXT: ret i16 %res
-define i16 @atomic_or_zero_seq_cst(i16* %addr) {
-  %res = atomicrmw or i16* %addr, i16 0 seq_cst
+define i16 @atomic_seq_cst(i16* %addr) {
+  %res = atomicrmw add i16* %addr, i16 0 seq_cst
   ret i16 %res
 }
 
 ; Check that the transformation does not apply when the value is changed by
 ; the atomic operation (non zero constant).
 ; CHECK-LABEL: atomic_add_non_zero
 ; CHECK-NEXT: %res = atomicrmw add i16* %addr, i16 2 monotonic
 ; CHECK-NEXT: ret i16 %res
 define i16 @atomic_add_non_zero(i16* %addr) {
   %res = atomicrmw add i16* %addr, i16 2 monotonic
   ret i16 %res
 }
 
 ; CHECK-LABEL: atomic_xor_zero
 ; CHECK-NEXT: %res = load atomic i16, i16* %addr monotonic, align 2
 ; CHECK-NEXT: ret i16 %res
 define i16 @atomic_xor_zero(i16* %addr) {
   %res = atomicrmw xor i16* %addr, i16 0 monotonic
   ret i16 %res
 }
 
 ; Check that the transformation does not apply when the ordering is
-; incompatible with a load (release).
-; CHECK-LABEL: atomic_or_zero_release
+; incompatible with a load (release).  Do canonicalize.
+; CHECK-LABEL: atomic_release
 ; CHECK-NEXT: %res = atomicrmw or i16* %addr, i16 0 release
 ; CHECK-NEXT: ret i16 %res
-define i16 @atomic_or_zero_release(i16* %addr) {
-  %res = atomicrmw or i16* %addr, i16 0 release
+define i16 @atomic_release(i16* %addr) {
+  %res = atomicrmw sub i16* %addr, i16 0 release
   ret i16 %res
 }
 
 ; Check that the transformation does not apply when the ordering is
-; incompatible with a load (acquire, release).
-; CHECK-LABEL: atomic_or_zero_acq_rel
+; incompatible with a load (acquire, release).  Do canonicalize.
+; CHECK-LABEL: atomic_acq_rel
 ; CHECK-NEXT: %res = atomicrmw or i16* %addr, i16 0 acq_rel
 ; CHECK-NEXT: ret i16 %res
-define i16 @atomic_or_zero_acq_rel(i16* %addr) {
-  %res = atomicrmw or i16* %addr, i16 0 acq_rel
+define i16 @atomic_acq_rel(i16* %addr) {
+  %res = atomicrmw xor i16* %addr, i16 0 acq_rel
   ret i16 %res
 }