From: Quentin Colombet Date: Wed, 18 Jan 2023 15:01:43 +0000 (+0100) Subject: [InstCombine] Don't optimize idempotent `atomicrmw , 0` into `load atomic` X-Git-Tag: upstream/17.0.6~20475 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6b85fa6d81b9f50d7a1c347dfd467c9ab01c4a5d;p=platform%2Fupstream%2Fllvm.git [InstCombine] Don't optimize idempotent `atomicrmw , 0` into `load atomic` Turning idempotent `atomicrmw`s 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. Differential Revision: https://reviews.llvm.org/D141277 --- diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp index 06d8fb2..e73667f 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp @@ -151,13 +151,5 @@ Instruction *InstCombinerImpl::visitAtomicRMWInst(AtomicRMWInst &RMWI) { 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; + return nullptr; } diff --git a/llvm/test/Transforms/InstCombine/atomicrmw.ll b/llvm/test/Transforms/InstCombine/atomicrmw.ll index 7da822b..d2789e4 100644 --- a/llvm/test/Transforms/InstCombine/atomicrmw.ll +++ b/llvm/test/Transforms/InstCombine/atomicrmw.ll @@ -3,12 +3,15 @@ ; Check that we can replace `atomicrmw LHS, 0` with `load atomic LHS`. ; This is possible when: -; - LHS, 0 == LHS -; - the ordering of atomicrmw is compatible with a load (i.e., no release semantic) +; Check that we don't replace `atomicrmw 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 @@ -17,70 +20,77 @@ define i32 @atomic_add_zero(ptr %addr) { 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 @@ -89,13 +99,14 @@ define float @atomic_fsub_zero(ptr %addr) { 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 @@ -126,9 +137,10 @@ define i64 @atomic_sub_zero_volatile(ptr %addr) { ; 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 @@ -157,9 +169,10 @@ define i16 @atomic_add_non_zero(ptr %addr) { 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