Skip to content

Commit

Permalink
[InstCombine] Don't replace unused atomicrmw xchg with atomic store
Browse files Browse the repository at this point in the history
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 #60418

Differential Revision: https://reviews.llvm.org/D142097
  • Loading branch information
qcolombet committed Feb 1, 2023
1 parent 87cd6f9 commit 583488e
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 19 deletions.
13 changes: 0 additions & 13 deletions llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
12 changes: 6 additions & 6 deletions llvm/test/Transforms/InstCombine/atomicrmw.ll
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 583488e

Please sign in to comment.