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

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 15 13:17:28 PST 2019

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
   return Load;
jfb wrote:
> I guess here you could have done `return eraseInstFromFunction(RMWI);` as well?
InstCombine does that automatically for non-void instructions.



More information about the llvm-commits mailing list