Pass QueryInst down through non-local dependency calculation

[reames]

Summary

This change is mostly motivated by exposing information about the original query instruction to the actual scanning work in getPointerDependencyFrom when used by GVN PRE. In a follow up change, I will use this to be more precise with regards to the semantics of volatile instructions encountered in the scan of a basic block.

Worth noting, is that this change (despite appearing quite simple) is not semantically preserving. By providing more information to the helper routine, we allow some optimizations to kick in that weren't previously able to (when called from this code path.) In particular, we see that treatment of !invariant.load becomes more precise. In theory, we might see a difference with an ordered/atomic instruction as well, but I'm having a hard time actually finding a test case which shows that.

Test wise, I've included new tests for !invariant.load which illustrate this difference.

I've also included some updated TBAA tests which highlight that this change isn't needed for that optimization to kick in - it's handled inside alias analysis itself. Arguably, these might call for being submitted separately. I don't have a strong preference either way and will defer to the reviewers.

Eventually, it would be nice to factor the !invariant.load handling inside alias analysis as well.

[reames]

ping

[reames]

ping x2

[hfinkel]

LGTM.

llvm/trunk/include/llvm/Analysis/MemoryDependenceAnalysis.h

static unsigned getLoadLoadClobberFullWidthSize(const Value *MemLocBase,
                                                     unsigned MemLocSize,
                                                     const LoadInst *LI,
                                                     const DataLayout &DL);
 
   private:
     MemDepResult getCallSiteDependencyFrom(CallSite C, bool isReadOnlyCall,
                                            BasicBlock::iterator ScanIt,
                                            BasicBlock *BB);
-    bool getNonLocalPointerDepFromBB(const PHITransAddr &Pointer,
+    bool getNonLocalPointerDepFromBB(Instruction *QueryInst,
+                                     const PHITransAddr &Pointer,
                                      const AliasAnalysis::Location &Loc,
                                      bool isLoad, BasicBlock *BB,
                                      SmallVectorImpl<NonLocalDepResult> &Result,
                                      DenseMap<BasicBlock*, Value*> &Visited,
                                      bool SkipFirstBlock = false);
-    MemDepResult GetNonLocalInfoForBlock(const AliasAnalysis::Location &Loc,
+    MemDepResult GetNonLocalInfoForBlock(Instruction *QueryInst,
+                                         const AliasAnalysis::Location &Loc,
                                          bool isLoad, BasicBlock *BB,
                                          NonLocalDepInfo *Cache,
                                          unsigned NumSortedEntries);
 
     void RemoveCachedNonLocalPointerDependencies(ValueIsLoadPair P);
 
     /// verifyRemoved - Verify that the specified instruction does not occur
     /// in our internal data structures.
     void verifyRemoved(Instruction *Inst) const;
 
   };
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp

getNonLocalPointerDependency(Instruction *QueryInst,
 
   PHITransAddr Address(const_cast<Value *>(Loc.Ptr), DL, AC);
 
   // This is the set of blocks we've inspected, and the pointer we consider in
   // each block.  Because of critical edges, we currently bail out if querying
   // a block with multiple different pointers.  This can happen during PHI
   // translation.
   DenseMap<BasicBlock*, Value*> Visited;
-  if (!getNonLocalPointerDepFromBB(Address, Loc, isLoad, FromBB,
+  if (!getNonLocalPointerDepFromBB(QueryInst, Address, Loc, isLoad, FromBB,
                                    Result, Visited, true))
     return;
   Result.clear();
   Result.push_back(NonLocalDepResult(FromBB,
                                      MemDepResult::getUnknown(),
                                      const_cast<Value *>(Loc.Ptr)));
 }
 
 /// GetNonLocalInfoForBlock - Compute the memdep value for BB with
 /// Pointer/PointeeSize using either cached information in Cache or by doing a
 /// lookup (which may use dirty cache info if available).  If we do a lookup,
 /// add the result to the cache.
 MemDepResult MemoryDependenceAnalysis::
-GetNonLocalInfoForBlock(const AliasAnalysis::Location &Loc,
+GetNonLocalInfoForBlock(Instruction *QueryInst,
+                        const AliasAnalysis::Location &Loc,
                         bool isLoad, BasicBlock *BB,
                         NonLocalDepInfo *Cache, unsigned NumSortedEntries) {
 
   // Do a binary search to see if we already have an entry for this block in
   // the cache set.  If so, find it.
   NonLocalDepInfo::iterator Entry =
     std::upper_bound(Cache->begin(), Cache->begin()+NumSortedEntries,
                      NonLocalDepEntry(BB));
if (ExistingResult && ExistingResult->getResult().getInst()) {
     // Eliminating the dirty entry from 'Cache', so update the reverse info.
     ValueIsLoadPair CacheKey(Loc.Ptr, isLoad);
     RemoveFromReverseMap(ReverseNonLocalPtrDeps, ScanPos, CacheKey);
   } else {
     ++NumUncacheNonLocalPtr;
   }
 
   // Scan the block for the dependency.
-  MemDepResult Dep = getPointerDependencyFrom(Loc, isLoad, ScanPos, BB);
+  MemDepResult Dep = getPointerDependencyFrom(Loc, isLoad, ScanPos, BB,
+                                              QueryInst);
 
   // If we had a dirty entry for the block, update it.  Otherwise, just add
   // a new entry.
   if (ExistingResult)
     ExistingResult->setResult(Dep);
   else
     Cache->push_back(NonLocalDepEntry(BB, Dep));
 

 /// This has special behavior for the first block queries (when SkipFirstBlock
 /// is true).  In this special case, it ignores the contents of the specified
 /// block and starts returning dependence info for its predecessors.
 ///
 /// This function returns false on success, or true to indicate that it could
 /// not compute dependence information for some reason.  This should be treated
 /// as a clobber dependence on the first instruction in the predecessor block.
 bool MemoryDependenceAnalysis::
-getNonLocalPointerDepFromBB(const PHITransAddr &Pointer,
+getNonLocalPointerDepFromBB(Instruction *QueryInst,
+                            const PHITransAddr &Pointer,
                             const AliasAnalysis::Location &Loc,
                             bool isLoad, BasicBlock *StartBB,
                             SmallVectorImpl<NonLocalDepResult> &Result,
                             DenseMap<BasicBlock*, Value*> &Visited,
                             bool SkipFirstBlock) {
   // Look up the cached info for Pointer.
   ValueIsLoadPair CacheKey(Pointer.getAddr(), isLoad);
 
if (CacheInfo->Size < Loc.Size) {
       for (NonLocalDepInfo::iterator DI = CacheInfo->NonLocalDeps.begin(),
            DE = CacheInfo->NonLocalDeps.end(); DI != DE; ++DI)
         if (Instruction *Inst = DI->getResult().getInst())
           RemoveFromReverseMap(ReverseNonLocalPtrDeps, Inst, CacheKey);
       CacheInfo->NonLocalDeps.clear();
     } else if (CacheInfo->Size > Loc.Size) {
       // This query's Size is less than the cached one. Conservatively restart
       // the query using the greater size.
-      return getNonLocalPointerDepFromBB(Pointer,
+      return getNonLocalPointerDepFromBB(QueryInst, Pointer,
                                          Loc.getWithNewSize(CacheInfo->Size),
                                          isLoad, StartBB, Result, Visited,
                                          SkipFirstBlock);
     }
 
     // If the query's AATags are inconsistent with the cached one,
     // conservatively throw out the cached data and restart the query with
     // no tag if needed.
     if (CacheInfo->AATags != Loc.AATags) {
       if (CacheInfo->AATags) {
         CacheInfo->Pair = BBSkipFirstBlockPair();
         CacheInfo->AATags = AAMDNodes();
         for (NonLocalDepInfo::iterator DI = CacheInfo->NonLocalDeps.begin(),
              DE = CacheInfo->NonLocalDeps.end(); DI != DE; ++DI)
           if (Instruction *Inst = DI->getResult().getInst())
             RemoveFromReverseMap(ReverseNonLocalPtrDeps, Inst, CacheKey);
         CacheInfo->NonLocalDeps.clear();
       }
       if (Loc.AATags)
-        return getNonLocalPointerDepFromBB(Pointer, Loc.getWithoutAATags(),
+        return getNonLocalPointerDepFromBB(QueryInst,
+                                           Pointer, Loc.getWithoutAATags(),
                                            isLoad, StartBB, Result, Visited,
                                            SkipFirstBlock);
     }
   }
 
   NonLocalDepInfo *Cache = &CacheInfo->NonLocalDeps;
 
   // If we have valid cached information for exactly the block we are
while (!Worklist.empty()) {
     if (!SkipFirstBlock) {
       // Analyze the dependency of *Pointer in FromBB.  See if we already have
       // been here.
       assert(Visited.count(BB) && "Should check 'visited' before adding to WL");
 
       // Get the dependency info for Pointer in BB.  If we have cached
       // information, we will use it, otherwise we compute it.
       DEBUG(AssertSorted(*Cache, NumSortedEntries));
-      MemDepResult Dep = GetNonLocalInfoForBlock(Loc, isLoad, BB, Cache,
+      MemDepResult Dep = GetNonLocalInfoForBlock(QueryInst,
+                                                 Loc, isLoad, BB, Cache,
                                                  NumSortedEntries);
 
       // If we got a Def or Clobber, add this to the list of results.
       if (!Dep.isNonLocal()) {
         if (!DT) {
           Result.push_back(NonLocalDepResult(BB,
                                              MemDepResult::getUnknown(),
                                              Pointer.getAddr()));
for (unsigned i = 0, n = PredList.size(); i < n; ++i) {
       // the same value.  Consider PHI translating something like:
       // X = phi [x, bb1], [y, bb2].  PHI translating for bb1 doesn't *need*
       // to recurse here, pedantically speaking.
 
       // If getNonLocalPointerDepFromBB fails here, that means the cached
       // result conflicted with the Visited list; we have to conservatively
       // assume it is unknown, but this also does not block PRE of the load.
       if (!CanTranslate ||
-          getNonLocalPointerDepFromBB(PredPointer,
+          getNonLocalPointerDepFromBB(QueryInst, PredPointer,
                                       Loc.getWithNewPtr(PredPtrVal),
                                       isLoad, Pred,
                                       Result, Visited)) {
         // Add the entry to the Result list.
         NonLocalDepResult Entry(Pred, MemDepResult::getUnknown(), PredPtrVal);
         Result.push_back(Entry);
 
         // Since we had a phi translation failure, the cache for CacheKey won't

llvm/trunk/test/Transforms/GVN/invariant-load.ll

entry:
   %x = load i32* %p, align 4
   %conv = trunc i32 %x to i8
   store i8 %conv, i8* %q, align 1
   %y = load i32* %p, align 4, !invariant.load !0
   %add = add i32 %y, 1
   ret i32 %add
 }
 
+; With the invariant.load metadata, what would otherwise
+; be a case for PRE becomes a full redundancy.
+define i32 @test3(i1 %cnd, i32* %p, i32* %q) {
+; CHECK-LABEL: test3
+; CHECK-NOT: load
+entry:
+  %v1 = load i32* %p
+  br i1 %cnd, label %bb1, label %bb2
+
+bb1:
+  store i32 5, i32* %q
+  br label %bb2
+
+bb2:
+  %v2 = load i32* %p, !invariant.load !0
+  %res = sub i32 %v1, %v2
+  ret i32 %res
+}
+
+; This test is here to document a case which doesn't optimize
+; as well as it could.  
+define i32 @test4(i1 %cnd, i32* %p, i32* %q) {
+; CHECK-LABEL: test4
+; %v2 is redundant, but GVN currently doesn't catch that
+entry:
+  %v1 = load i32* %p, !invariant.load !0
+  br i1 %cnd, label %bb1, label %bb2
+
+bb1:
+  store i32 5, i32* %q
+  br label %bb2
+
+bb2:
+  %v2 = load i32* %p
+  %res = sub i32 %v1, %v2
+  ret i32 %res
+}
+
 !0 = !{ }
 

llvm/trunk/test/Transforms/GVN/tbaa.ll

-; RUN: opt -basicaa -gvn -S < %s | FileCheck %s
+; RUN: opt -tbaa -basicaa -gvn -S < %s | FileCheck %s
 
 define i32 @test1(i8* %p, i8* %q) {
 ; CHECK: @test1(i8* %p, i8* %q)
 ; CHECK: call i32 @foo(i8* %p)
 ; CHECK-NOT: tbaa
 ; CHECK: %c = add i32 %a, %a
   %a = call i32 @foo(i8* %p), !tbaa !0
   %b = call i32 @foo(i8* %p)

 ; CHECK-NOT: tbaa
 ; CHECK: %c = add i32 %a, %a
   %a = call i32 @foo(i8* %p), !tbaa !4
   %b = call i32 @foo(i8* %p), !tbaa !3
   %c = add i32 %a, %b
   ret i32 %c
 }
 
+
+
+define i32 @test8(i32* %p, i32* %q) {
+; CHECK-LABEL: test8
+; CHECK-NEXT: store i32 15, i32* %p
+; CHECK-NEXT: ret i32 0
+; Since we know the location is invariant, we can forward the
+; load across the potentially aliasing store.
+
+  %a = load i32* %q, !tbaa !10
+  store i32 15, i32* %p
+  %b = load i32* %q, !tbaa !10
+  %c = sub i32 %a, %b
+  ret i32 %c
+}
+define i32 @test9(i32* %p, i32* %q) {
+; CHECK-LABEL: test9
+; CHECK-NEXT: call void @clobber()
+; CHECK-NEXT: ret i32 0
+; Since we know the location is invariant, we can forward the
+; load across the potentially aliasing store (within the call).
+
+  %a = load i32* %q, !tbaa !10
+  call void @clobber()
+  %b = load i32* %q, !tbaa !10
+  %c = sub i32 %a, %b
+  ret i32 %c
+}
+
+
+declare void @clobber()
 declare i32 @foo(i8*) readonly
 
 ; CHECK: [[TAGC]] = !{[[TYPEC:!.*]], [[TYPEC]], i64 0}
 ; CHECK: [[TYPEC]] = !{!"C", [[TYPEA:!.*]]}
 ; CHECK: [[TYPEA]] = !{!"A", !{{.*}}}
 ; CHECK: [[TAGB]] = !{[[TYPEB:!.*]], [[TYPEB]], i64 0}
 ; CHECK: [[TYPEB]] = !{!"B", [[TYPEA]]}
 ; CHECK: [[TAGA]] = !{[[TYPEA]], [[TYPEA]], i64 0}
 !0 = !{!5, !5, i64 0}
 !1 = !{!6, !6, i64 0}
 !2 = !{!"tbaa root", null}
 !3 = !{!7, !7, i64 0}
 !4 = !{!8, !8, i64 0}
 !5 = !{!"C", !6}
 !6 = !{!"A", !2}
 !7 = !{!"B", !6}
 !8 = !{!"another root", null}
+
+
+;; A TBAA structure who's only point is to have a constant location
+!9 = !{!"yet another root"}
+!10 = !{!"node", !9, i64 1}
+