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

JF Bastien via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 15 09:55:16 PST 2019

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
   return Load;
I guess here you could have done `return eraseInstFromFunction(RMWI);` as well?



More information about the llvm-commits mailing list