[PATCH] D58290: Convert atomicrmws to xchg or store where legal

[JF Bastien via Phabricator via llvm-commits]

jfb accepted this revision.
jfb added a comment.
This revision is now accepted and ready to land.

A few nits, LGTM otherwise.



================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:52
+/// Return true if the given instruction always produces a value in memory
+/// equivelent to it's value operand.
+bool isSaturating(AtomicRMWInst& RMWI) {
----------------
"its value's"


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:59
+  AtomicRMWInst::BinOp Op = RMWI.getOperation();
+  switch(Op) {
+  default:
----------------
You can move `RMWI.getOperation()` in here.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:73
+  case AtomicRMWInst::UMax:
+    return C->isMaxValue(false);
+  };
----------------
`FAdd` / `FSub` with any NaN are also saturating.

Technically `xchg` is also saturating, pre your description above...


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:99
+  // Any atomicrmw xchg with no uses can be converted to a atomic store if the
+  // ordering is compatible. 
+  if (RMWI.getOperation() == AtomicRMWInst::Xchg &&
----------------
Do you want to factor this out and run it after `isSaturating` succeeds above? That way instcombine doesn't have to do two passes to do this transform.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:133
   Load->setAlignment(DL.getABITypeAlignment(RMWI.getType()));
   return Load;
 }
----------------
I guess here you could have done `return eraseInstFromFunction(RMWI);` as well?


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

https://reviews.llvm.org/D58290