From ac5b42cad850ee9c934812f543cdb139cc7d9e98 Mon Sep 17 00:00:00 2001 From: David Majnemer Date: Sat, 21 Mar 2015 06:19:17 +0000 Subject: [PATCH] MemoryDependenceAnalysis: Don't miscompile atomics r216771 introduced a change to MemoryDependenceAnalysis that allowed it to reason about acquire/release operations. However, this change does not ensure that the acquire/release operations pair. Unfortunately, this leads to miscompiles as we won't see an acquire load as properly memory effecting. This largely reverts r216771. This fixes PR22708. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@232889 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Analysis/MemoryDependenceAnalysis.cpp | 15 ++--- .../Transforms/DeadStoreElimination/atomic.ll | 46 --------------- test/Transforms/GVN/atomic.ll | 56 ++++++------------- 3 files changed, 21 insertions(+), 96 deletions(-) diff --git a/lib/Analysis/MemoryDependenceAnalysis.cpp b/lib/Analysis/MemoryDependenceAnalysis.cpp index 8fdca0be353..ab293932da5 100644 --- a/lib/Analysis/MemoryDependenceAnalysis.cpp +++ b/lib/Analysis/MemoryDependenceAnalysis.cpp @@ -407,7 +407,6 @@ getPointerDependencyFrom(const AliasAnalysis::Location &MemLoc, bool isLoad, // by every program that can detect any optimisation of that kind: either // it is racy (undefined) or there is a release followed by an acquire // between the pair of accesses under consideration. - bool HasSeenAcquire = false; if (isLoad && QueryInst) { LoadInst *LI = dyn_cast(QueryInst); @@ -468,12 +467,12 @@ getPointerDependencyFrom(const AliasAnalysis::Location &MemLoc, bool isLoad, // Atomic loads have complications involved. // A Monotonic (or higher) load is OK if the query inst is itself not atomic. - // An Acquire (or higher) load sets the HasSeenAcquire flag, so that any - // release store will know to return getClobber. // FIXME: This is overly conservative. if (LI->isAtomic() && LI->getOrdering() > Unordered) { if (!QueryInst) return MemDepResult::getClobber(LI); + if (LI->getOrdering() != Monotonic) + return MemDepResult::getClobber(LI); if (auto *QueryLI = dyn_cast(QueryInst)) { if (!QueryLI->isSimple()) return MemDepResult::getClobber(LI); @@ -483,9 +482,6 @@ getPointerDependencyFrom(const AliasAnalysis::Location &MemLoc, bool isLoad, } else if (QueryInst->mayReadOrWriteMemory()) { return MemDepResult::getClobber(LI); } - - if (isAtLeastAcquire(LI->getOrdering())) - HasSeenAcquire = true; } AliasAnalysis::Location LoadLoc = AA->getLocation(LI); @@ -545,12 +541,12 @@ getPointerDependencyFrom(const AliasAnalysis::Location &MemLoc, bool isLoad, if (StoreInst *SI = dyn_cast(Inst)) { // Atomic stores have complications involved. // A Monotonic store is OK if the query inst is itself not atomic. - // A Release (or higher) store further requires that no acquire load - // has been seen. // FIXME: This is overly conservative. if (!SI->isUnordered()) { if (!QueryInst) return MemDepResult::getClobber(SI); + if (SI->getOrdering() != Monotonic) + return MemDepResult::getClobber(SI); if (auto *QueryLI = dyn_cast(QueryInst)) { if (!QueryLI->isSimple()) return MemDepResult::getClobber(SI); @@ -560,9 +556,6 @@ getPointerDependencyFrom(const AliasAnalysis::Location &MemLoc, bool isLoad, } else if (QueryInst->mayReadOrWriteMemory()) { return MemDepResult::getClobber(SI); } - - if (HasSeenAcquire && isAtLeastRelease(SI->getOrdering())) - return MemDepResult::getClobber(SI); } // FIXME: this is overly conservative. diff --git a/test/Transforms/DeadStoreElimination/atomic.ll b/test/Transforms/DeadStoreElimination/atomic.ll index 4d2cb37f258..79beee803e4 100644 --- a/test/Transforms/DeadStoreElimination/atomic.ll +++ b/test/Transforms/DeadStoreElimination/atomic.ll @@ -23,28 +23,6 @@ define void @test1() { ret void } -; DSE across seq_cst load (allowed) -define i32 @test2() { -; CHECK-LABEL: test2 -; CHECK-NOT: store i32 0 -; CHECK: store i32 1 - store i32 0, i32* @x - %x = load atomic i32, i32* @y seq_cst, align 4 - store i32 1, i32* @x - ret i32 %x -} - -; DSE across seq_cst store (allowed) -define void @test3() { -; CHECK-LABEL: test3 -; CHECK-NOT: store i32 0 -; CHECK: store atomic i32 2 - store i32 0, i32* @x - store atomic i32 2, i32* @y seq_cst, align 4 - store i32 1, i32* @x - ret void -} - ; DSE remove unordered store (allowed) define void @test4() { ; CHECK-LABEL: test4 @@ -141,30 +119,6 @@ define void @test12() { ret void } -; DSE is allowed across a pair of an atomic read and then write. -define i32 @test13() { -; CHECK-LABEL: test13 -; CHECK-NOT: store i32 0 -; CHECK: store i32 1 - store i32 0, i32* @x - %x = load atomic i32, i32* @y seq_cst, align 4 - store atomic i32 %x, i32* @y seq_cst, align 4 - store i32 1, i32* @x - ret i32 %x -} - -; Same if it is acquire-release instead of seq_cst/seq_cst -define i32 @test14() { -; CHECK-LABEL: test14 -; CHECK-NOT: store i32 0 -; CHECK: store i32 1 - store i32 0, i32* @x - %x = load atomic i32, i32* @y acquire, align 4 - store atomic i32 %x, i32* @y release, align 4 - store i32 1, i32* @x - ret i32 %x -} - ; But DSE is not allowed across a release-acquire pair. define i32 @test15() { ; CHECK-LABEL: test15 diff --git a/test/Transforms/GVN/atomic.ll b/test/Transforms/GVN/atomic.ll index aada1378ee2..11b54f39756 100644 --- a/test/Transforms/GVN/atomic.ll +++ b/test/Transforms/GVN/atomic.ll @@ -18,18 +18,6 @@ entry: ret i32 %z } -; GVN across seq_cst store (allowed) -define i32 @test2() nounwind uwtable ssp { -; CHECK-LABEL: test2 -; CHECK: add i32 %x, %x -entry: - %x = load i32, i32* @y - store atomic i32 %x, i32* @x seq_cst, align 4 - %y = load i32, i32* @y - %z = add i32 %x, %y - ret i32 %z -} - ; GVN across unordered load (allowed) define i32 @test3() nounwind uwtable ssp { ; CHECK-LABEL: test3 @@ -43,20 +31,6 @@ entry: ret i32 %b } -; GVN across acquire load (allowed as the original load was not atomic) -define i32 @test4() nounwind uwtable ssp { -; CHECK-LABEL: test4 -; CHECK: load atomic i32, i32* @x -; CHECK-NOT: load i32, i32* @y -entry: - %x = load i32, i32* @y - %y = load atomic i32, i32* @x seq_cst, align 4 - %x2 = load i32, i32* @y - %x3 = add i32 %x, %x2 - %y2 = add i32 %y, %x3 - ret i32 %y2 -} - ; GVN load to unordered load (allowed) define i32 @test5() nounwind uwtable ssp { ; CHECK-LABEL: test5 @@ -92,19 +66,6 @@ entry: ret i32 %z } -; GVN across acquire-release pair (allowed) -define i32 @test8() nounwind uwtable ssp { -; CHECK-LABEL: test8 -; CHECK: add i32 %x, %x -entry: - %x = load i32, i32* @y - %w = load atomic i32, i32* @x acquire, align 4 - store atomic i32 %x, i32* @x release, align 4 - %y = load i32, i32* @y - %z = add i32 %x, %y - ret i32 %z -} - ; GVN across monotonic store (allowed) define i32 @test9() nounwind uwtable ssp { ; CHECK-LABEL: test9 @@ -129,3 +90,20 @@ entry: ret i32 %z } +define i32 @PR22708(i1 %flag) { +; CHECK-LABEL: PR22708 +entry: + br i1 %flag, label %if.then, label %if.end + +if.then: + store i32 43, i32* @y, align 4 +; CHECK: store i32 43, i32* @y, align 4 + br label %if.end + +if.end: + load atomic i32, i32* @x acquire, align 4 + %load = load i32, i32* @y, align 4 +; CHECK: load atomic i32, i32* @x acquire, align 4 +; CHECK: load i32, i32* @y, align 4 + ret i32 %load +} -- 2.34.1