From: David Majnemer <david.majnemer@gmail.com>
Date: Sat, 21 Mar 2015 06:19:17 +0000 (+0000)
Subject: MemoryDependenceAnalysis: Don't miscompile atomics
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=ac5b42cad850ee9c934812f543cdb139cc7d9e98;p=oota-llvm.git

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
---

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<LoadInst>(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<LoadInst>(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<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 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
+}