[InstCombine] Don't optimize idempotent `atomicrmw <op>, 0` into `load atomic`

[qcolombet]

Summary

Turning idempotent atomicrmws into load atomic is perfectly legal
with respect to how the loading happens, but it may not be legal for the
whole program semantic.

Indeed, this optimization removes a store that may have some effects on
the legality of other optimizations.
Essentially, we lose some information and depending on the backend
it may or may not produce incorrect code, so don't do it!

This fixes llvm.org/PR56450.

[nikic]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

Just to double check, is this atomicrmw -> store fold fine, or does it also have a similar issue?

[qcolombet]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

It might, but I'd like a "language lawyer" to confirm.

It took me some time to get convinced that this optimization is wrong to begin with so I would rather have an actual expert on the matter to comment :).

@RalfJung

[RalfJung]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

I'm afraid I am not a weak memory expert, so I don't know for sure.

But given that RMWs have special semantics that stores don't have, I am at least doubtful of this optimization. I will ask some other people. :)
If I understand the code correctly, this will apply specifically to Xchg whose loaded value is ignored, and whose ordering is Release or Relaxed?

[RalfJung]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

Judging from the discussion at https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/LLVM.20atomic.20optimization.3A.20atomic.20swap.20to.20atomic.20store, the optimization is indeed wrong since it breaks release sequences. If LLVM wants to perform such optimizations, at the very least it would have to define its own weak memory model that is very distinct from the C++ one -- and carefully justify why translating from C++ to that model is correct.

[qcolombet]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

@reames what do you think?

From what @RalfJung is reporting (thanks Ralf BTW), the change that replaces xchg with atomic store is technically incorrect.
I.e., #2 from https://reviews.llvm.org/D58290.

I'm happy to remove that change in that PR too.
Otherwise I would suggest we move with at least the current fix since it produces miscompiles in the wild.

[nikic]

LGTM, might as well just land this part first.

[arsenm]

Can you just insert a fence to go with the load?

[arsenm]

In D141277#4060040, @arsenm wrote:

Can you just insert a fence to go with the load?

AtomicExpand also tries the same transform (which for some reason is done in an x86 specific way, so this might be incomplete anyway)

[qcolombet]

Rebase

[qcolombet]

AtomicExpand also tries the same transform (which for some reason is done in an x86 specific way, so this might be incomplete anyway)

Yeah, I believe there would be some subtle issues doing that in a generic way.

[nikic]

llvm/test/Transforms/InstCombine/atomicrmw.ll

5–6

I think the two comment lines above were supposed to be dropped.

[qcolombet]

llvm/test/Transforms/InstCombine/atomicrmw.ll

5–6

Good catch, looks like I screwed up the merge conflict.

[qcolombet]

llvm/test/Transforms/InstCombine/atomicrmw.ll

5–6

Fixed in ffafa0d43d81

[qcolombet]

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

134

Patch for this particular issue is at https://reviews.llvm.org/D142097

llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp

Instruction *InstCombinerImpl::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
   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() &&
       RMWI.getOperation() != AtomicRMWInst::Or) {
     RMWI.setOperation(AtomicRMWInst::Or);
     return replaceOperand(RMWI, 1, ConstantInt::get(RMWI.getType(), 0));
   } else if (RMWI.getType()->isFloatingPointTy() &&
              RMWI.getOperation() != AtomicRMWInst::FAdd) {
     RMWI.setOperation(AtomicRMWInst::FAdd);
     return replaceOperand(RMWI, 1, ConstantFP::getNegativeZero(RMWI.getType()));
   }
 
-  // Check if the required ordering is compatible with an atomic load.
-  if (Ordering != AtomicOrdering::Acquire &&
-      Ordering != AtomicOrdering::Monotonic)
-    return nullptr;
-
-  LoadInst *Load = new LoadInst(RMWI.getType(), RMWI.getPointerOperand(), "",
-                                false, DL.getABITypeAlign(RMWI.getType()),
-                                Ordering, RMWI.getSyncScopeID());
-  return Load;
 }

llvm/test/Transforms/InstCombine/atomicrmw.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes=instcombine -S -o - %s | FileCheck %s
 
 ; Check that we can replace `atomicrmw <op> LHS, 0` with `load atomic LHS`.
 ; This is possible when:
-; - <op> LHS, 0 == LHS
-; - the ordering of atomicrmw is compatible with a load (i.e., no release semantic)
+; Check that we don't replace `atomicrmw <op> LHS, 0` with `load atomic LHS`.
+; Doing that would lose the store semantic of the `atomicrmw` operation.
+; This may enable some other optimizations that would otherwise be illegal when
+; the store semantic was present (e.g., like dropping a fence).
 
+; Idempotent atomicrmw are still canonicalized.
 define i32 @atomic_add_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_add_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw add ptr %addr, i32 0 monotonic
   ret i32 %res
 }
 
 define i32 @atomic_or_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_or_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw add ptr %addr, i32 0 monotonic
   ret i32 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i32 @atomic_sub_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_sub_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw sub ptr %addr, i32 0 monotonic
   ret i32 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i32 @atomic_and_allones(ptr %addr) {
 ; CHECK-LABEL: @atomic_and_allones(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw and ptr %addr, i32 -1 monotonic
   ret i32 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i32 @atomic_umin_uint_max(ptr %addr) {
 ; CHECK-LABEL: @atomic_umin_uint_max(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw umin ptr %addr, i32 -1 monotonic
   ret i32 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i32 @atomic_umax_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_umax_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i32, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i32 0 monotonic, align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %res = atomicrmw umax ptr %addr, i32 0 monotonic
   ret i32 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i8 @atomic_min_smax_char(ptr %addr) {
 ; CHECK-LABEL: @atomic_min_smax_char(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i8, ptr [[ADDR:%.*]] monotonic, align 1
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i8 0 monotonic, align 1
 ; CHECK-NEXT:    ret i8 [[RES]]
 ;
   %res = atomicrmw min ptr %addr, i8 127 monotonic
   ret i8 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i8 @atomic_max_smin_char(ptr %addr) {
 ; CHECK-LABEL: @atomic_max_smin_char(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i8, ptr [[ADDR:%.*]] monotonic, align 1
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i8 0 monotonic, align 1
 ; CHECK-NEXT:    ret i8 [[RES]]
 ;
   %res = atomicrmw max ptr %addr, i8 -128 monotonic
   ret i8 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define float @atomic_fsub_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_fsub_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic float, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw fadd ptr [[ADDR:%.*]], float -0.000000e+00 monotonic, align 4
 ; CHECK-NEXT:    ret float [[RES]]
 ;
   %res = atomicrmw fsub ptr %addr, float 0.0 monotonic
   ret float %res
 }
 
 define float @atomic_fadd_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_fadd_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic float, ptr [[ADDR:%.*]] monotonic, align 4
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw fadd ptr [[ADDR:%.*]], float -0.000000e+00 monotonic, align 4
 ; CHECK-NEXT:    ret float [[RES]]
 ;
   %res = atomicrmw fadd ptr %addr, float -0.0 monotonic
   ret float %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define float @atomic_fsub_canon(ptr %addr) {
 ; CHECK-LABEL: @atomic_fsub_canon(
 ; CHECK-NEXT:    [[RES:%.*]] = atomicrmw fadd ptr [[ADDR:%.*]], float -0.000000e+00 release, align 4
 ; CHECK-NEXT:    ret float [[RES]]
 ;
   %res = atomicrmw fsub ptr %addr, float 0.0 release
   ret float %res
 }

 ; CHECK-NEXT:    ret i64 [[RES]]
 ;
   %res = atomicrmw volatile sub ptr %addr, i64 0 acquire
   ret i64 %res
 }
 
 
 ; Check that the transformation properly preserve the syncscope.
+; Idempotent atomicrmw are still canonicalized.
 define i16 @atomic_syncscope(ptr %addr) {
 ; CHECK-LABEL: @atomic_syncscope(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i16, ptr [[ADDR:%.*]] syncscope("some_syncscope") acquire, align 2
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i16 0 syncscope("some_syncscope") acquire, align 2
 ; CHECK-NEXT:    ret i16 [[RES]]
 ;
   %res = atomicrmw or ptr %addr, i16 0 syncscope("some_syncscope") acquire
   ret i16 %res
 }
 
 ; By eliminating the store part of the atomicrmw, we would get rid of the
 ; release semantic, which is incorrect.  We can canonicalize the operation.

 ; CHECK-LABEL: @atomic_add_non_zero(
 ; CHECK-NEXT:    [[RES:%.*]] = atomicrmw add ptr [[ADDR:%.*]], i16 2 monotonic, align 2
 ; CHECK-NEXT:    ret i16 [[RES]]
 ;
   %res = atomicrmw add ptr %addr, i16 2 monotonic
   ret i16 %res
 }
 
+; Idempotent atomicrmw are still canonicalized.
 define i16 @atomic_xor_zero(ptr %addr) {
 ; CHECK-LABEL: @atomic_xor_zero(
-; CHECK-NEXT:    [[RES:%.*]] = load atomic i16, ptr [[ADDR:%.*]] monotonic, align 2
+; CHECK-NEXT:    [[RES:%.*]] = atomicrmw or ptr [[ADDR:%.*]], i16 0 monotonic, align 2
 ; CHECK-NEXT:    ret i16 [[RES]]
 ;
   %res = atomicrmw xor ptr %addr, i16 0 monotonic
   ret i16 %res
 }
 
 ; Check that the transformation does not apply when the ordering is
 ; incompatible with a load (release).  Do canonicalize.