[PATCH] D70103: [WIP][LazyValueInfo] Fix non-determinism in cache.

[Eli Friedman via Phabricator via llvm-commits]

efriedma created this revision.
efriedma added a reviewer: nikic.
Herald added subscribers: mgrang, hiraditya.
Herald added a project: LLVM.

OverDefinedCache contains a set of values that are overdefined in a given basic block.  However, we don't reliably update the set when one of the values is erased.

This is really two fixes: one, add a PoisoningVH to catch the issue, and two, fix the resulting issue.

I'm posting this now for two reasons: one, to verify this actually fixes https://bugs.llvm.org/show_bug.cgi?id=43909, and two, to ask for ideas on how to improve the algorithm. The current implementation is really inefficient, but I'm not sure what the best approach looks like.

I haven't reduced the testcase yet.

Fixes https://bugs.llvm.org/show_bug.cgi?id=43909 (hopefully).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70103

Files:
  llvm/lib/Analysis/LazyValueInfo.cpp


Index: llvm/lib/Analysis/LazyValueInfo.cpp
===================================================================
--- llvm/lib/Analysis/LazyValueInfo.cpp
+++ llvm/lib/Analysis/LazyValueInfo.cpp
@@ -152,7 +152,7 @@
   /// maintains information about queries across the clients' queries.
   class LazyValueInfoCache {
   public:
-    typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>>
+    typedef DenseMap<PoisoningVH<BasicBlock>, DenseSet<PoisoningVH<Value>>>
         PerBlockValueCacheTy;
 
   private:
@@ -181,6 +181,7 @@
     /// dereferenced in the block (and thus non-null at the end of the block).
     PerBlockValueCacheTy DereferencedPointerCache;
 
+    std::vector<LVIValueHandle> OverdefinedHandles;
 
   public:
     void insertResult(Value *Val, BasicBlock *BB,
@@ -189,9 +190,12 @@
 
       // Insert over-defined values into their own cache to reduce memory
       // overhead.
-      if (Result.isOverdefined())
+      if (Result.isOverdefined()) {
         OverDefinedCache[BB].insert(Val);
-      else {
+        auto I = llvm::find_if(OverdefinedHandles, [Val](auto &P) { return P == Val; });
+        if (I == OverdefinedHandles.end())
+          OverdefinedHandles.emplace_back(Val, this);
+      } else {
         auto It = ValueCache.find_as(Val);
         if (It == ValueCache.end()) {
           ValueCache[Val] = std::make_unique<ValueCacheEntryTy>(Val, this);
@@ -270,7 +274,7 @@
     // Copy and increment the iterator immediately so we can erase behind
     // ourselves.
     auto Iter = I++;
-    SmallPtrSetImpl<Value *> &ValueSet = Iter->second;
+    auto &ValueSet = Iter->second;
     ValueSet.erase(V);
     if (ValueSet.empty())
       Cache.erase(Iter);
@@ -281,6 +285,10 @@
   eraseValueFromPerBlockValueCache(V, OverDefinedCache);
   eraseValueFromPerBlockValueCache(V, DereferencedPointerCache);
   ValueCache.erase(V);
+  auto I = llvm::find_if(OverdefinedHandles, [V](auto &P) { return P == V; });
+  if (I != OverdefinedHandles.end())
+    OverdefinedHandles.erase(I);
+
 }
 
 void LVIValueHandle::deleted() {
@@ -341,7 +349,7 @@
     auto OI = OverDefinedCache.find(ToUpdate);
     if (OI == OverDefinedCache.end())
       continue;
-    SmallPtrSetImpl<Value *> &ValueSet = OI->second;
+    auto &ValueSet = OI->second;
 
     bool changed = false;
     for (Value *V : ValsToClear) {
@@ -685,7 +693,7 @@
 }
 
 static void AddDereferencedPointer(
-    Value *Ptr, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {
+    Value *Ptr, DenseSet<PoisoningVH<Value>> &PtrSet, const DataLayout &DL) {
   // TODO: Use NullPointerIsDefined instead.
   if (Ptr->getType()->getPointerAddressSpace() == 0) {
     Ptr = GetUnderlyingObject(Ptr, DL);
@@ -694,7 +702,7 @@
 }
 
 static void AddPointersDereferencedByInstruction(
-    Instruction *I, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {
+    Instruction *I, DenseSet<PoisoningVH<Value>> &PtrSet, const DataLayout &DL) {
   if (LoadInst *L = dyn_cast<LoadInst>(I)) {
     AddDereferencedPointer(L->getPointerOperand(), PtrSet, DL);
   } else if (StoreInst *S = dyn_cast<StoreInst>(I)) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70103.228779.patch
Type: text/x-patch
Size: 3106 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191112/d87452f4/attachment.bin>