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

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:59
+  AtomicRMWInst::BinOp Op = RMWI.getOperation();
+  switch(Op) {
+  default:
----------------
jfb wrote:
> You can move `RMWI.getOperation()` in here.
Will do this and the other site of the same in a follow up nfc


================
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 &&
----------------
jfb wrote:
> 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.
Saving a cycle through the iteration didn't seem worth code complexity.  


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


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

https://reviews.llvm.org/D58290