[llvm] 583488e - [InstCombine] Don't replace unused `atomicrmw xchg` with `atomic store`

[Quentin Colombet via llvm-commits]

Author: Quentin Colombet
Date: 2023-02-01T11:48:43+01:00
New Revision: 583488e4a787e400b6077b64677d23855a3c9538

URL: https://github.com/llvm/llvm-project/commit/583488e4a787e400b6077b64677d23855a3c9538
DIFF: https://github.com/llvm/llvm-project/commit/583488e4a787e400b6077b64677d23855a3c9538.diff

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

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

This fixes https://github.com/llvm/llvm-project/issues/60418

Differential Revision: https://reviews.llvm.org/D142097

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
index e73667f9c02ea..7b0860b6942d5 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
@@ -121,19 +121,6 @@ Instruction *InstCombinerImpl::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
          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;
 

diff  --git a/llvm/test/Transforms/InstCombine/atomicrmw.ll b/llvm/test/Transforms/InstCombine/atomicrmw.ll
index cb2ebf729575a..0c35904265611 100644
--- a/llvm/test/Transforms/InstCombine/atomicrmw.ll
+++ b/llvm/test/Transforms/InstCombine/atomicrmw.ll
@@ -273,7 +273,7 @@ define double @sat_fsub_nan(ptr %addr) {
 
 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 @sat_fsub_nan_unused(ptr %addr) {
 
 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_monotonic(ptr %addr) {
 
 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_release(ptr %addr) {
 
 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_under_aligned(ptr %addr) {
 
 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 @xchg_unused_volatile(ptr %addr) {
 
 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