Allow None as a MemoryLocation to getModRefInfo

[asbirlea]

Summary

Adding part of the changes in D30369 (needed to make progress):
Current patch updates AliasAnalysis and MemoryLocation, but does _not_ clean up MemorySSA.

Original summary from D30369, by dberlin:
Currently, we have instructions which affect memory but have no memory
location. If you call, for example, MemoryLocation::get on a fence,
it asserts. This means things specifically have to avoid that. It
also means we end up with a copy of each API, one taking a memory
location, one not.

This starts to fix that.

We add MemoryLocation::getOrNone as a new call, and reimplement the
old asserting version in terms of it.

We make MemoryLocation optional in the (Instruction, MemoryLocation)
version of getModRefInfo, and kill the old one argument version in
favor of passing None (it had one caller). Now both can handle fences
because you can just use MemoryLocation::getOrNone on an instruction
and it will return a correct answer.

We use all this to clean up part of MemorySSA that had to handle this difference.

Note that literally every actual getModRefInfo interface we have could be made private and replaced with:

getModRefInfo(Instruction, Optional<MemoryLocation>)
and
getModRefInfo(Instruction, Optional<MemoryLocation>, Instruction, Optional<MemoryLocation>)

and delegating to the right ones, if we wanted to.

I have not attempted to do this yet.

[• dberlin]

So, this is a subset of an already approved patch, so LGTM.

[davide]

Sorry, I thought I already LGTM'd this one (I definitely did on the original)

include/llvm/Analysis/AliasAnalysis.h

 //    MustAlias at the same time. The current API can only return one result,
 //    though this is rarely a problem in practice.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_ALIASANALYSIS_H
 #define LLVM_ANALYSIS_ALIASANALYSIS_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 class BasicAAResult;
public:
   ModRefInfo getModRefInfo(const CatchReturnInst *I, const MemoryLocation &Loc);
 
   /// getModRefInfo (for catchrets) - A convenience wrapper.
   ModRefInfo getModRefInfo(const CatchReturnInst *I, const Value *P,
                            uint64_t Size) {
     return getModRefInfo(I, MemoryLocation(P, Size));
   }
 
-  /// Check whether or not an instruction may read or write memory (without
-  /// regard to a specific location).
+  /// Check whether or not an instruction may read or write the optionally
+  /// specified memory location.
+  ///
+  ///
+  /// An instruction that doesn't read or write memory may be trivially LICM'd
+  /// for example.
   ///
   /// For function calls, this delegates to the alias-analysis specific
-  /// call-site mod-ref behavior queries. Otherwise it delegates to the generic
-  /// mod ref information query without a location.
-  ModRefInfo getModRefInfo(const Instruction *I) {
+  /// call-site mod-ref behavior queries. Otherwise it delegates to the specific
+  /// helpers above.
+  ModRefInfo getModRefInfo(const Instruction *I,
+                           const Optional<MemoryLocation> &OptLoc) {
+    if (OptLoc == None) {
       if (auto CS = ImmutableCallSite(I)) {
         auto MRB = getModRefBehavior(CS);
         if ((MRB & MRI_ModRef) == MRI_ModRef)
           return MRI_ModRef;
         if (MRB & MRI_Ref)
           return MRI_Ref;
         if (MRB & MRI_Mod)
           return MRI_Mod;
         return MRI_NoModRef;
       }
-
-    return getModRefInfo(I, MemoryLocation());
     }
 
-  /// 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.
-  ///
-  /// This primarily delegates to specific helpers above.
-  ModRefInfo getModRefInfo(const Instruction *I, const MemoryLocation &Loc) {
+    const MemoryLocation &Loc = OptLoc.getValueOr(MemoryLocation());
+
     switch (I->getOpcode()) {
     case Instruction::VAArg:  return getModRefInfo((const VAArgInst*)I, Loc);
     case Instruction::Load:   return getModRefInfo((const LoadInst*)I,  Loc);
     case Instruction::Store:  return getModRefInfo((const StoreInst*)I, Loc);
     case Instruction::Fence:  return getModRefInfo((const FenceInst*)I, Loc);
     case Instruction::AtomicCmpXchg:
       return getModRefInfo((const AtomicCmpXchgInst*)I, Loc);
     case Instruction::AtomicRMW:

include/llvm/Analysis/MemoryLocation.h

 /// These are used both by the Alias Analysis infrastructure and more
 /// specialized memory analysis layers.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_MEMORYLOCATION_H
 #define LLVM_ANALYSIS_MEMORYLOCATION_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Metadata.h"
 
 namespace llvm {
 
 class LoadInst;
 class StoreInst;
public:
   /// Return a location with information about the memory reference by the given
   /// instruction.
   static MemoryLocation get(const LoadInst *LI);
   static MemoryLocation get(const StoreInst *SI);
   static MemoryLocation get(const VAArgInst *VI);
   static MemoryLocation get(const AtomicCmpXchgInst *CXI);
   static MemoryLocation get(const AtomicRMWInst *RMWI);
   static MemoryLocation get(const Instruction *Inst) {
-    if (auto *I = dyn_cast<LoadInst>(Inst))
-      return get(I);
-    else if (auto *I = dyn_cast<StoreInst>(Inst))
-      return get(I);
-    else if (auto *I = dyn_cast<VAArgInst>(Inst))
-      return get(I);
-    else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))
-      return get(I);
-    else if (auto *I = dyn_cast<AtomicRMWInst>(Inst))
-      return get(I);
-    llvm_unreachable("unsupported memory instruction");
+    return *MemoryLocation::getOrNone(Inst);
+  }
+  static Optional<MemoryLocation> getOrNone(const Instruction *Inst) {
+    switch (Inst->getOpcode()) {
+    case Instruction::Load:
+      return get(cast<LoadInst>(Inst));
+    case Instruction::Store:
+      return get(cast<StoreInst>(Inst));
+    case Instruction::VAArg:
+      return get(cast<VAArgInst>(Inst));
+    case Instruction::AtomicCmpXchg:
+      return get(cast<AtomicCmpXchgInst>(Inst));
+    case Instruction::AtomicRMW:
+      return get(cast<AtomicRMWInst>(Inst));
+    default:
+      return None;
+    }
   }
 
   /// Return a location representing the source of a memory transfer.
   static MemoryLocation getForSource(const MemTransferInst *MTI);
 
   /// Return a location representing the destination of a memory set or
   /// transfer.
   static MemoryLocation getForDest(const MemIntrinsic *MI);

lib/Analysis/MemorySSA.cpp

MemoryUseOrDef *MemorySSA::createNewAccess(Instruction *I) {
   // that it writes arbitrarily. Ignore that fake memory dependency here.
   // FIXME: Replace this special casing with a more accurate modelling of
   // assume's control dependency.
   if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
     if (II->getIntrinsicID() == Intrinsic::assume)
       return nullptr;
 
   // Find out what affect this instruction has on memory.
-  ModRefInfo ModRef = AA->getModRefInfo(I);
+  ModRefInfo ModRef = AA->getModRefInfo(I, None);
   // 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".

lib/Transforms/Scalar/MemCpyOptimizer.cpp

static bool moveUp(AliasAnalysis &AA, StoreInst *SI, Instruction *P,
   // Lifted callsites.
   SmallVector<ImmutableCallSite, 8> CallSites;
 
   const MemoryLocation LoadLoc = MemoryLocation::get(LI);
 
   for (auto I = --SI->getIterator(), E = P->getIterator(); I != E; --I) {
     auto *C = &*I;
 
-    bool MayAlias = AA.getModRefInfo(C) != MRI_NoModRef;
+    bool MayAlias = AA.getModRefInfo(C, MemoryLocation()) != MRI_NoModRef;
 
     bool NeedLift = false;
     if (Args.erase(C))
       NeedLift = true;
     else if (MayAlias) {
       NeedLift = llvm::any_of(MemLocs, [C, &AA](const MemoryLocation &ML) {
         return AA.getModRefInfo(C, ML);
       });

unittests/Analysis/AliasAnalysisTest.cpp

auto *AtomicRMW =
                         AtomicOrdering::Monotonic, SyncScope::System, BB);
 
   ReturnInst::Create(C, nullptr, BB);
 
   auto &AA = getAAResults(*F);
 
   // Check basic results
   EXPECT_EQ(AA.getModRefInfo(Store1, MemoryLocation()), MRI_Mod);
-  EXPECT_EQ(AA.getModRefInfo(Store1), MRI_Mod);
+  EXPECT_EQ(AA.getModRefInfo(Store1, None), MRI_Mod);
   EXPECT_EQ(AA.getModRefInfo(Load1, MemoryLocation()), MRI_Ref);
-  EXPECT_EQ(AA.getModRefInfo(Load1), MRI_Ref);
+  EXPECT_EQ(AA.getModRefInfo(Load1, None), MRI_Ref);
   EXPECT_EQ(AA.getModRefInfo(Add1, MemoryLocation()), MRI_NoModRef);
-  EXPECT_EQ(AA.getModRefInfo(Add1), MRI_NoModRef);
+  EXPECT_EQ(AA.getModRefInfo(Add1, None), MRI_NoModRef);
   EXPECT_EQ(AA.getModRefInfo(VAArg1, MemoryLocation()), MRI_ModRef);
-  EXPECT_EQ(AA.getModRefInfo(VAArg1), MRI_ModRef);
+  EXPECT_EQ(AA.getModRefInfo(VAArg1, None), MRI_ModRef);
   EXPECT_EQ(AA.getModRefInfo(CmpXChg1, MemoryLocation()), MRI_ModRef);
-  EXPECT_EQ(AA.getModRefInfo(CmpXChg1), MRI_ModRef);
+  EXPECT_EQ(AA.getModRefInfo(CmpXChg1, None), MRI_ModRef);
   EXPECT_EQ(AA.getModRefInfo(AtomicRMW, MemoryLocation()), MRI_ModRef);
-  EXPECT_EQ(AA.getModRefInfo(AtomicRMW), MRI_ModRef);
+  EXPECT_EQ(AA.getModRefInfo(AtomicRMW, None), MRI_ModRef);
 }
 
 class AAPassInfraTest : public testing::Test {
 protected:
   LLVMContext C;
   SMDiagnostic Err;
   std::unique_ptr<Module> M;