[llvm] r232889 - MemoryDependenceAnalysis: Don't miscompile atomics
David Majnemer
david.majnemer at gmail.com
Fri Mar 20 23:19:17 PDT 2015
Author: majnemer
Date: Sat Mar 21 01:19:17 2015
New Revision: 232889
URL: http://llvm.org/viewvc/llvm-project?rev=232889&view=rev
Log:
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.
Modified:
llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
llvm/trunk/test/Transforms/GVN/atomic.ll
Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=232889&r1=232888&r2=232889&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp (original)
+++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Sat Mar 21 01:19:17 2015
@@ -407,7 +407,6 @@ getPointerDependencyFrom(const AliasAnal
// 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<LoadInst>(QueryInst);
@@ -468,12 +467,12 @@ getPointerDependencyFrom(const AliasAnal
// 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<LoadInst>(QueryInst)) {
if (!QueryLI->isSimple())
return MemDepResult::getClobber(LI);
@@ -483,9 +482,6 @@ getPointerDependencyFrom(const AliasAnal
} 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 AliasAnal
if (StoreInst *SI = dyn_cast<StoreInst>(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<LoadInst>(QueryInst)) {
if (!QueryLI->isSimple())
return MemDepResult::getClobber(SI);
@@ -560,9 +556,6 @@ getPointerDependencyFrom(const AliasAnal
} else if (QueryInst->mayReadOrWriteMemory()) {
return MemDepResult::getClobber(SI);
}
-
- if (HasSeenAcquire && isAtLeastRelease(SI->getOrdering()))
- return MemDepResult::getClobber(SI);
}
// FIXME: this is overly conservative.
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll (original)
+++ llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll Sat Mar 21 01:19:17 2015
@@ -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
Modified: llvm/trunk/test/Transforms/GVN/atomic.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/atomic.ll (original)
+++ llvm/trunk/test/Transforms/GVN/atomic.ll Sat Mar 21 01:19:17 2015
@@ -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
+}
More information about the llvm-commits
mailing list