[llvm] r299741 - AliasAnalysis: Be less conservative about volatile than atomic.

[Daniel Berlin via llvm-commits]

Author: dannyb
Date: Thu Apr  6 20:28:36 2017
New Revision: 299741

URL: http://llvm.org/viewvc/llvm-project?rev=299741&view=rev
Log:
AliasAnalysis: Be less conservative about volatile than atomic.

Summary:
getModRefInfo is meant to answer the question "what impact does this
instruction have on a given memory location" (not even another
instruction).

Long debate on this on IRC comes to the conclusion the answer should be "nothing special".

That is, a noalias volatile store does not affect a memory location
just by being volatile.  Note: DSE and GVN and memdep currently
believe this, because memdep just goes behind AA's back after it says
"modref" right now.

see line 635 of memdep. Prior to this patch we would get modref there, then check aliasing,
and if it said noalias, we would continue.

getModRefInfo *already* has this same AA check, it just wasn't being used because volatile was
lumped in with ordering.

(I am separately testing whether this code in memdep is now dead except for the invariant load case)

Reviewers: jyknight, chandlerc

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D31726

Modified:
    llvm/trunk/include/llvm/Analysis/AliasAnalysis.h
    llvm/trunk/lib/Analysis/AliasAnalysis.cpp
    llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
    llvm/trunk/test/Transforms/NewGVN/volatile-nonvolatile.ll

Modified: llvm/trunk/include/llvm/Analysis/AliasAnalysis.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/AliasAnalysis.h?rev=299741&r1=299740&r2=299741&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Analysis/AliasAnalysis.h (original)
+++ llvm/trunk/include/llvm/Analysis/AliasAnalysis.h Thu Apr  6 20:28:36 2017
@@ -524,6 +524,14 @@ public:
   /// Check whether or not an instruction may read or write the specified
   /// memory location.
   ///
+  /// Note explicitly that getModRefInfo considers the effects of reading and
+  /// writing the memory location, and not the effect of ordering relative to
+  /// other instructions.  Thus, a volatile load is considered to be Ref,
+  /// because it does not actually write memory, it just can't be reordered
+  /// relative to other volatiles (or removed).  Atomic ordered loads/stores are
+  /// considered ModRef ATM because conservatively, the visible effect appears
+  /// as if memory was written, not just an ordering constraint.
+  ///
   /// An instruction that doesn't read or write memory may be trivially LICM'd
   /// for example.
   ///

Modified: llvm/trunk/lib/Analysis/AliasAnalysis.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/AliasAnalysis.cpp?rev=299741&r1=299740&r2=299741&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/AliasAnalysis.cpp (original)
+++ llvm/trunk/lib/Analysis/AliasAnalysis.cpp Thu Apr  6 20:28:36 2017
@@ -332,8 +332,8 @@ FunctionModRefBehavior AAResults::getMod
 
 ModRefInfo AAResults::getModRefInfo(const LoadInst *L,
                                     const MemoryLocation &Loc) {
-  // Be conservative in the face of volatile/atomic.
-  if (!L->isUnordered())
+  // Be conservative in the face of atomic.
+  if (isStrongerThan(L->getOrdering(), AtomicOrdering::Unordered))
     return MRI_ModRef;
 
   // If the load address doesn't alias the given address, it doesn't read
@@ -347,8 +347,8 @@ ModRefInfo AAResults::getModRefInfo(cons
 
 ModRefInfo AAResults::getModRefInfo(const StoreInst *S,
                                     const MemoryLocation &Loc) {
-  // Be conservative in the face of volatile/atomic.
-  if (!S->isUnordered())
+  // Be conservative in the face of atomic.
+  if (isStrongerThan(S->getOrdering(), AtomicOrdering::Unordered))
     return MRI_ModRef;
 
   if (Loc.Ptr) {

Modified: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp?rev=299741&r1=299740&r2=299741&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp Thu Apr  6 20:28:36 2017
@@ -849,7 +849,8 @@ struct RenamePassData {
 } // anonymous namespace
 
 namespace llvm {
-/// \brief A MemorySSAWalker that does AA walks to disambiguate accesses. It no longer does caching on its own,
+/// \brief A MemorySSAWalker that does AA walks to disambiguate accesses. It no
+/// longer does caching on its own,
 /// but the name has been retained for the moment.
 class MemorySSA::CachingWalker final : public MemorySSAWalker {
   ClobberWalker Walker;
@@ -1456,6 +1457,19 @@ MemoryUseOrDef *MemorySSA::createDefined
   return NewAccess;
 }
 
+// Return true if the instruction has ordering constraints.
+// Note specifically that this only considers stores and loads
+// because others are still considered ModRef by getModRefInfo.
+static inline bool isOrdered(const Instruction *I) {
+  if (auto *SI = dyn_cast<StoreInst>(I)) {
+    if (!SI->isUnordered())
+      return true;
+  } else if (auto *LI = dyn_cast<LoadInst>(I)) {
+    if (!LI->isUnordered())
+      return true;
+  }
+  return false;
+}
 /// \brief Helper function to create new memory accesses
 MemoryUseOrDef *MemorySSA::createNewAccess(Instruction *I) {
   // The assume intrinsic has a control dependency which we model by claiming
@@ -1468,7 +1482,15 @@ MemoryUseOrDef *MemorySSA::createNewAcce
 
   // Find out what affect this instruction has on memory.
   ModRefInfo ModRef = AA->getModRefInfo(I);
-  bool Def = bool(ModRef & MRI_Mod);
+  // The isOrdered check is used to ensure that volatiles end up as defs
+  // (atomics end up as ModRef right now anyway).  Until we separate the
+  // ordering chain from the memory chain, this enables people to see at least
+  // some relative ordering to volatiles.  Note that getClobberingMemoryAccess
+  // will still give an answer that bypasses other volatile loads.  TODO:
+  // Separate memory aliasing and ordering into two different chains so that we
+  // can precisely represent both "what memory will this read/write/is clobbered
+  // by" and "what instructions can I move this past".
+  bool Def = bool(ModRef & MRI_Mod) || isOrdered(I);
   bool Use = bool(ModRef & MRI_Ref);
 
   // It's possible for an instruction to not modify memory at all. During

Modified: llvm/trunk/test/Transforms/NewGVN/volatile-nonvolatile.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/volatile-nonvolatile.ll?rev=299741&r1=299740&r2=299741&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/volatile-nonvolatile.ll (original)
+++ llvm/trunk/test/Transforms/NewGVN/volatile-nonvolatile.ll Thu Apr  6 20:28:36 2017
@@ -1,4 +1,3 @@
-; XFAIL: *
 ; RUN: opt -tbaa -newgvn -S < %s | FileCheck %s
 
 %struct.t = type { i32* }