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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked 3 inline comments as done.
qcolombet 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
----------------
jfb wrote:
> I don't think you should modify `volatile` this way.
Good point, I'm killing the volatile store in the process, which is invalid.


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


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57854