[InstCombine] Don't replace unused `atomicrmw xchg` with `atomic store`

[qcolombet]

Summary

Following the discussion from https://reviews.llvm.org/D141277 and in
particular Ralf Jung's comment at
https://reviews.llvm.org/D141277#inline-1365148, replacing an unused `atomicrmw
xchg` into an atomic store is illegal even for release ordering.

Quoting Connor Horman from the rust lang discussion linked in that comment:
"An acquire operation A only synchronizes-with a release operation R if it
takes its value from R, or any store in the release sequence headed by R, which
is R, followed by the longest continuous sequence of read-modify-write
operations.
A regular store following R in the modification order would break the release
sequence, and if an acquire operation reads that store or something later, then
it loses any synchronization it might have already had."

[qcolombet]

Ping

[RalfJung]

I agree this optimization should not be performed. Also see https://github.com/llvm/llvm-project/issues/60418 for a counterexample.

[nikic]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

if (isSaturating(RMWI) &&
     return &RMWI;
   }
 
   AtomicOrdering Ordering = RMWI.getOrdering();
   assert(Ordering != AtomicOrdering::NotAtomic &&
          Ordering != AtomicOrdering::Unordered &&
          "AtomicRMWs don't make sense with Unordered or NotAtomic");
 
-  // Any atomicrmw xchg with no uses can be converted to a atomic store if the
-  // ordering is compatible.
-  if (RMWI.getOperation() == AtomicRMWInst::Xchg &&
-      RMWI.use_empty()) {
-    if (Ordering != AtomicOrdering::Release &&
-        Ordering != AtomicOrdering::Monotonic)
-      return nullptr;
-    new StoreInst(RMWI.getValOperand(), RMWI.getPointerOperand(),
-                  /*isVolatile*/ false, RMWI.getAlign(), Ordering,
-                  RMWI.getSyncScopeID(), &RMWI);
-    return eraseInstFromFunction(RMWI);
-  }
-
   if (!isIdempotentRMW(RMWI))
     return nullptr;
 
   // We chose to canonicalize all idempotent operations to an single
   // operation code and constant.  This makes it easier for the rest of the
   // optimizer to match easily.  The choices of or w/0 and fadd w/-0.0 are
   // arbitrary.
   if (RMWI.getType()->isIntegerTy() &&

llvm/test/Transforms/InstCombine/atomicrmw.ll

 ; CHECK-NEXT:    ret double [[RES]]
 ;
   %res = atomicrmw fsub ptr %addr, double 0x7FF00000FFFFFFFF release
   ret double %res
 }
 
 define void @sat_fsub_nan_unused(ptr %addr) {
 ; CHECK-LABEL: @sat_fsub_nan_unused(
-; CHECK-NEXT:    store atomic double 0x7FF00000FFFFFFFF, ptr [[ADDR:%.*]] monotonic, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], double 0x7FF00000FFFFFFFF monotonic, align 8
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw fsub ptr %addr, double 0x7FF00000FFFFFFFF monotonic
   ret void
 }
 
 define void @xchg_unused_monotonic(ptr %addr) {
 ; CHECK-LABEL: @xchg_unused_monotonic(
-; CHECK-NEXT:    store atomic i32 0, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw xchg ptr %addr, i32 0 monotonic
   ret void
 }
 
 define void @xchg_unused_release(ptr %addr) {
 ; CHECK-LABEL: @xchg_unused_release(
-; CHECK-NEXT:    store atomic i32 -1, ptr [[ADDR:%.*]] release, align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], i32 -1 release, align 4
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw xchg ptr %addr, i32 -1 release
   ret void
 }
 
 define void @xchg_unused_under_aligned(ptr %addr) {
 ; CHECK-LABEL: @xchg_unused_under_aligned(
-; CHECK-NEXT:    store atomic i32 -1, ptr [[ADDR:%.*]] release, align 1
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], i32 -1 release, align 1
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw xchg ptr %addr, i32 -1 release, align 1
   ret void
 }
 
 define void @xchg_unused_over_aligned(ptr %addr) {
 ; CHECK-LABEL: @xchg_unused_over_aligned(
-; CHECK-NEXT:    store atomic i32 -1, ptr [[ADDR:%.*]] release, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], i32 -1 release, align 8
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw xchg ptr %addr, i32 -1 release, align 8
   ret void
 }
 
 define void @xchg_unused_seq_cst(ptr %addr) {
 ; CHECK-LABEL: @xchg_unused_seq_cst(

 ; CHECK-NEXT:    ret void
 ;
   atomicrmw volatile xchg ptr %addr, i32 0 monotonic
   ret void
 }
 
 define void @sat_or_allones_unused(ptr %addr) {
 ; CHECK-LABEL: @sat_or_allones_unused(
-; CHECK-NEXT:    store atomic i32 -1, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr [[ADDR:%.*]], i32 -1 monotonic, align 4
 ; CHECK-NEXT:    ret void
 ;
   atomicrmw or ptr %addr, i32 -1 monotonic
   ret void
 }
 
 define void @undef_operand_unused(ptr %addr) {
 ; CHECK-LABEL: @undef_operand_unused(