[PATCH] D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:27
+    if (!CI || !CI->isZero())
+      break;
+    // Check if the required ordering is compatible with an
----------------
I don't think you should modify `volatile` this way.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:32
+    if (Ordering == AtomicOrdering::Release ||
+        Ordering == AtomicOrdering::AcquireRelease)
+      break;
----------------
This doesn't capture seq_cst. I think you want to list the valid ones: NotAtomic / Unordered / Monotonic / Acquire.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:36
+    Load->setVolatile(RMWI.isVolatile());
+    Load->setAtomic(Ordering, RMWI.getSyncScopeID());
+    return Load;
----------------
This drops SyncScope.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57854