RPCS3/llvm-mirror
1
0
Fork 0
mirror of https://github.com/RPCS3/llvm-mirror.git synced 2024-11-22 02:33:06 +01:00
Code Issues Projects Releases Wiki Activity

[SimplifyCFG] Improve store speculation check

Browse Source 
isSafeToSpeculateStore() looks for a preceding store to the same
location to make sure that introducing a new store of the same
value is safe. It currently bails on intervening mayHaveSideEffect()
instructions. However, I believe just checking mayWriteToMemory()
is sufficient there -- we just need to make sure that we know which
value was stored, we don't care if we can unwind in the meantime.

While looking into this, I started having some doubts about the
correctness of the transform with regard to thread safety. While
we don't try to hoist non-simple stores, I believe we also need
to make sure that the preceding store is simple as well. Otherwise
we could introduce a spurious non-atomic write after an atomic write
-- under our memory model this would result in a subsequent undef
atomic read, even if the second write stores the same value as the
first.

Example: https://alive2.llvm.org/ce/z/q_3YAL

Differential Revision: https://reviews.llvm.org/D106742
This commit is contained in:
Nikita Popov 2021-07-21 22:34:28 +02:00
parent d43867c3c3
commit 845ad210b0
2 changed files with 13 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
lib/Transforms/Utils/SimplifyCFG.cpp
View File

@ -2240,13 +2240,15 @@ static Value *isSafeToSpeculateStore(Instruction *I, BasicBlock *BrBB,
--MaxNumInstToLookAt;
// Could be calling an instruction that affects memory like free().
if (CurI.mayHaveSideEffects() && !isa<StoreInst>(CurI))
if (CurI.mayWriteToMemory() && !isa<StoreInst>(CurI))
return nullptr;
if (auto *SI = dyn_cast<StoreInst>(&CurI)) {
// Found the previous store make sure it stores to the same location.
// Found the previous store to same location and type. Make sure it is
// simple, to avoid introducing a spurious non-atomic write after an
// atomic write.
if (SI->getPointerOperand() == StorePtr &&
SI->getValueOperand()->getType() == StoreTy)
SI->getValueOperand()->getType() == StoreTy && SI->isSimple())
// Found the previous store, return its value operand.
return SI->getValueOperand();
return nullptr; // Unknown store.

16
test/Transforms/SimplifyCFG/speculate-store.ll
View File

@ -135,13 +135,11 @@ ret.end:
define void @readonly_call(ptr %ptr, i1 %cmp) {
; CHECK-LABEL: @readonly_call(
; CHECK-NEXT: ret.end:
; CHECK-NEXT: store i32 0, ptr [[PTR:%.*]], align 4
; CHECK-NEXT: call void @unknown_fun() #[[ATTR0:[0-9]+]]
; CHECK-NEXT: br i1 [[CMP:%.*]], label [[IF_THEN:%.*]], label [[RET_END:%.*]]
; CHECK: if.then:
; CHECK-NEXT: store i32 1, ptr [[PTR]], align 4
; CHECK-NEXT: br label [[RET_END]]
; CHECK: ret.end:
; CHECK-NEXT: [[SPEC_STORE_SELECT:%.*]] = select i1 [[CMP:%.*]], i32 1, i32 0
; CHECK-NEXT: store i32 [[SPEC_STORE_SELECT]], ptr [[PTR]], align 4
; CHECK-NEXT: ret void
;
store i32 0, ptr %ptr
@ -158,10 +156,12 @@ ret.end:
define void @atomic_and_simple(ptr %ptr, i1 %cmp) {
; CHECK-LABEL: @atomic_and_simple(
; CHECK-NEXT: ret.end:
; CHECK-NEXT: store atomic i32 0, ptr [[PTR:%.*]] seq_cst, align 4
; CHECK-NEXT: [[SPEC_STORE_SELECT:%.*]] = select i1 [[CMP:%.*]], i32 1, i32 0
; CHECK-NEXT: store i32 [[SPEC_STORE_SELECT]], ptr [[PTR]], align 4
; CHECK-NEXT: br i1 [[CMP:%.*]], label [[IF_THEN:%.*]], label [[RET_END:%.*]]
; CHECK: if.then:
; CHECK-NEXT: store i32 1, ptr [[PTR]], align 4
; CHECK-NEXT: br label [[RET_END]]
; CHECK: ret.end:
; CHECK-NEXT: ret void
;
store atomic i32 0, ptr %ptr seq_cst, align 4