[Refactor] Have getNonLocalPointerDependency take the query instruction

[reames]

Summary

Previously, MemoryDependenceAnalysis::getNonLocalPointerDependency was taking a list of properties about the instruction being queried. Since I'm about to need one more property to be passed down through the infrastructure - I need to know a query instruction is non-volatile in an inner helper - fix the interface once and for all.

I also added some assertions and behaviour clarifications around volatile and ordered field accesses. At the moment, this is mostly to document expected behaviour. The only non-standard instructions which can current reach this are atomic, but unordered, loads and stores. Neither ordered or volatile accesses can reach here.

The call in GVN is protected by an isSimple check when it first considers the load. The calls in MemDepPrinter are protected by isUnordered checks. Both utilities also check isVolatile for loads and stores.

Review wise, I'm just looking for style comments and a second look to make sure I didn't typo something and accidentally change behaviour.

[sanjoy]

The only thing that concerns me is the new alternate return path on if (is_ordered(QueryInst)) {. With that addressed, LGTM.

include/llvm/Analysis/MemoryDependenceAnalysis.h

373

What is BB here?

lib/Analysis/MemoryDependenceAnalysis.cpp

866

Minor & optional: I'd just assert(isa<LoadInst>(I) || isa<StoreInst>(I) ...) and then return AA->getLocation(I); because that makes it obvious that the result is always AA->getLocation(I).

899

Shouldn't this be isOrdered? Or does llvm allow non camelcase for lambdas?

908

Where did this come from? Is this a semantic difference?

[reames]

Thanks for the comments. I'll fix and submit.

include/llvm/Analysis/MemoryDependenceAnalysis.h

373

Fixed.

lib/Analysis/MemoryDependenceAnalysis.cpp

866

AA::getLocation(Instruction *I) doesn't exists. I may move this lambda to be that function, but that would be a separate change.

899

Not sure actually, but I took your suggestion.

908

Technically, yes. I'll replace it with the assert - which is not a change - before submitting. My next change is to pull in some of the logic from MemDepPrinter into this function.

[reames]

This was closed with 225481; I forgot to include the link.

include/llvm/Analysis/MemoryDependenceAnalysis.h

 
 
     /// getNonLocalPointerDependency - Perform a full dependency query for an
-    /// access to the specified (non-volatile) memory location, returning the
-    /// set of instructions that either define or clobber the value.
+    /// access to the QueryInst's specified (non-volatile) memory location,
+    /// returning the set of instructions that either define or clobber
+    /// the value.
     ///
     /// This method assumes the pointer has a "NonLocal" dependency within BB.
-    void getNonLocalPointerDependency(const AliasAnalysis::Location &Loc,
-                                      bool isLoad, BasicBlock *BB,
+    void getNonLocalPointerDependency(Instruction *QueryInst,
                                     SmallVectorImpl<NonLocalDepResult> &Result);
 
     /// removeInstruction - Remove an instruction from the dependence analysis,

lib/Analysis/MemDepPrinter.cpp

 
 bool MemDepPrinter::runOnFunction(Function &F) {
   this->F = &F;
-  AliasAnalysis &AA = getAnalysis<AliasAnalysis>();
   MemoryDependenceAnalysis &MDA = getAnalysis<MemoryDependenceAnalysis>();
 
   // All this code uses non-const interfaces because MemDep is not

                                            static_cast<BasicBlock *>(nullptr)));
           continue;
         }
-        AliasAnalysis::Location Loc = AA.getLocation(LI);
-        MDA.getNonLocalPointerDependency(Loc, true, LI->getParent(), NLDI);
+        MDA.getNonLocalPointerDependency(LI, NLDI);
       } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
         if (!SI->isUnordered()) {
           // FIXME: Handle atomic/volatile stores.

                                            static_cast<BasicBlock *>(nullptr)));
           continue;
         }
-        AliasAnalysis::Location Loc = AA.getLocation(SI);
-        MDA.getNonLocalPointerDependency(Loc, false, SI->getParent(), NLDI);
+        MDA.getNonLocalPointerDependency(SI, NLDI);
       } else if (VAArgInst *VI = dyn_cast<VAArgInst>(Inst)) {
-        AliasAnalysis::Location Loc = AA.getLocation(VI);
-        MDA.getNonLocalPointerDependency(Loc, false, VI->getParent(), NLDI);
+        MDA.getNonLocalPointerDependency(VI, NLDI);
       } else {
         llvm_unreachable("Unknown memory instruction!");
       }

lib/Analysis/MemoryDependenceAnalysis.cpp

 /// own block.
 ///
 void MemoryDependenceAnalysis::
-getNonLocalPointerDependency(const AliasAnalysis::Location &Loc, bool isLoad,
-                             BasicBlock *FromBB,
+getNonLocalPointerDependency(Instruction *QueryInst,
                              SmallVectorImpl<NonLocalDepResult> &Result) {
+
+  auto getLocation = [](AliasAnalysis *AA, Instruction *Inst) {
+    if (auto *I = dyn_cast<LoadInst>(Inst))
+      return AA->getLocation(I);
+    else if (auto *I = dyn_cast<StoreInst>(Inst))
+      return AA->getLocation(I);
+    else if (auto *I = dyn_cast<VAArgInst>(Inst))
+      return AA->getLocation(I);
+    else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))
+      return AA->getLocation(I);
+    else if (auto *I = dyn_cast<AtomicRMWInst>(Inst))
+      return AA->getLocation(I);
+    else
+      llvm_unreachable("unsupported memory instruction");
+  };
+   
+  const AliasAnalysis::Location Loc = getLocation(AA, QueryInst);
+  bool isLoad = isa<LoadInst>(QueryInst);
+  BasicBlock *FromBB = QueryInst->getParent();
+  assert(FromBB);
+  
+  // This routine does not expect to deal with volatile instructions.  Doing so
+  // would require piping through the QueryInst all the way through.
+  // TODO: volatiles can't be elided, but they can be reordered with other
+  // non-volatile accesses.  
+  if (LoadInst *LI = dyn_cast<LoadInst>(QueryInst)) {
+    assert(!LI->isVolatile());
+  } else if (StoreInst *SI = dyn_cast<StoreInst>(QueryInst)) {
+    assert(!SI->isVolatile());
+  }
+
+
+  // We currently give up on any instruction which is ordered, but we do handle
+  // atomic instructions which are unordered.
+  // TODO: Handle ordered instructions
+  auto is_ordered = [](Instruction *Inst) {
+    if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
+      return !LI->isUnordered();
+    } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
+      return !SI->isUnordered();
+    }
+    return false;
+  };
+
+  if (is_ordered(QueryInst)) {
+    Result.push_back(NonLocalDepResult(FromBB,
+                                       MemDepResult::getUnknown(),
+                                       const_cast<Value *>(Loc.Ptr)));
+    return;
+  }
+
   assert(Loc.Ptr->getType()->isPointerTy() &&
          "Can't get pointer deps of a non-pointer!");
   Result.clear();

lib/Transforms/Scalar/GVN.cpp

 bool GVN::processNonLocalLoad(LoadInst *LI) {
   // Step 1: Find the non-local dependencies of the load.
   LoadDepVect Deps;
-  AliasAnalysis::Location Loc = VN.getAliasAnalysis()->getLocation(LI);
-  MD->getNonLocalPointerDependency(Loc, true, LI->getParent(), Deps);
+  MD->getNonLocalPointerDependency(LI, Deps);
 
   // If we had to process more than one hundred blocks to find the
   // dependencies, this load isn't worth worrying about.  Optimizing