[PATCH] D64405: [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses. Fix for https://bugs.llvm.org/show_bug.cgi?id=42151

[Evgeniy via Phabricator via llvm-commits]

ebrevnov created this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Dependence anlysis has a mechanism to cache results. Thus for particular memory access the cache keep track of side effects in basic blocks. The problem is that for invariant loads dependepce analysis legally ignores many dependencies due to a special semantic rules for such loads. But later results calculated for invariant load retrived from the cache for general case acceses. As a result we have wrong dependence information causing GVN to do illegal transformation. Fixes, T42151.

Proposed solution is to disable caching of invariant loads. I think such loads a pretty rare and it doesn't make sense to extend caching mechanism for them.


Repository:
  rL LLVM

https://reviews.llvm.org/D64405

Files:
  lib/Analysis/MemoryDependenceAnalysis.cpp


Index: lib/Analysis/MemoryDependenceAnalysis.cpp
===================================================================
--- lib/Analysis/MemoryDependenceAnalysis.cpp
+++ lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -978,22 +978,33 @@
     Instruction *QueryInst, const MemoryLocation &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 (Entry != Cache->begin() && (Entry - 1)->getBB() == BB)
-    --Entry;
-
   NonLocalDepEntry *ExistingResult = nullptr;
-  if (Entry != Cache->begin() + NumSortedEntries && Entry->getBB() == BB)
-    ExistingResult = &*Entry;
-
-  // If we have a cached entry, and it is non-dirty, use it as the value for
-  // this dependency.
-  if (ExistingResult && !ExistingResult->getResult().isDirty()) {
-    ++NumCacheNonLocalPtr;
-    return ExistingResult->getResult();
+  bool isInvariantLoad = false;
+
+  if (LoadInst *LI = dyn_cast_or_null<LoadInst>(QueryInst))
+      isInvariantLoad = LI->getMetadata(LLVMContext::MD_invariant_load);
+
+  // Due to a semantic of invariant load many dependencies are ignored. Thus
+  // results of dependence analysis can't be shared with general case.
+  // Don't use cached results for invariant load.
+  if (!isInvariantLoad) {
+    // 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 (Entry != Cache->begin() && (Entry - 1)->getBB() == BB)
+      --Entry;
+
+    if (Entry != Cache->begin() + NumSortedEntries && Entry->getBB() == BB)
+      ExistingResult = &*Entry;
+
+    // If we have a cached entry, and it is non-dirty, use it as the value for
+    // this dependency.
+    if (ExistingResult && !ExistingResult->getResult().isDirty()) {
+      ++NumCacheNonLocalPtr;
+      return ExistingResult->getResult();
+    }
   }
 
   // Otherwise, we have to scan for the value.  If we have a dirty cache
@@ -1017,12 +1028,14 @@
   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));
+  // Don't put results of dependence analysis into cache.
+  if (!isInvariantLoad)
+    // 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));
 
   // If the block has a dependency (i.e. it isn't completely transparent to
   // the value), remember the reverse association because we just added it


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64405.208645.patch
Type: text/x-patch
Size: 3109 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190709/e825c1f9/attachment.bin>