[PATCH] D58244: Canonicalize all "idempotent" atomicrmw ops

[Philip Reames via Phabricator via llvm-commits]

reames updated this revision to Diff 186883.
reames marked an inline comment as done.
reames added a comment.

Address review comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58244/new/

https://reviews.llvm.org/D58244

Files:
  lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
  test/Transforms/InstCombine/atomicrmw.ll


Index: test/Transforms/InstCombine/atomicrmw.ll
===================================================================
--- test/Transforms/InstCombine/atomicrmw.ll
+++ test/Transforms/InstCombine/atomicrmw.ll
@@ -87,14 +87,13 @@
   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
 }
 
@@ -117,21 +116,21 @@
 }
 
 ; 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
 }
Index: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
+++ lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
@@ -54,14 +54,21 @@
   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.
+  // this by just a load. We chose not to canonicalize because...?
   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 &&


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58244.186883.patch
Type: text/x-patch
Size: 3211 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190214/3cbcdb14/attachment.bin>