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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet created this revision.
qcolombet added reviewers: reames, jfb, radialbuttonhox, nikic.
Herald added subscribers: StephenFan, JDevlieghere, hiraditya.
Herald added a project: All.
qcolombet requested review of this revision.
Herald added a project: LLVM.

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."


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142097

Files:
  llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
  llvm/test/Transforms/InstCombine/atomicrmw.ll


Index: llvm/test/Transforms/InstCombine/atomicrmw.ll
===================================================================
--- llvm/test/Transforms/InstCombine/atomicrmw.ll
+++ llvm/test/Transforms/InstCombine/atomicrmw.ll
@@ -273,7 +273,7 @@
 
 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
@@ -282,7 +282,7 @@
 
 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
@@ -291,7 +291,7 @@
 
 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
@@ -300,7 +300,7 @@
 
 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
@@ -309,7 +309,7 @@
 
 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
@@ -336,7 +336,7 @@
 
 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
Index: llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
@@ -121,19 +121,6 @@
          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;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142097.490430.patch
Type: text/x-patch
Size: 3397 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230119/6b90cdf6/attachment.bin>